Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Slightly improved heuristic to find the target root #24

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

micampe
Copy link
Contributor

@micampe micampe commented Oct 7, 2023

Since I don't have any files in my target's root directory (only other subdirectories), the current heuristic can't find my sql file, because it only looks at those subdirectories, so it picks a random one (the first in the list).

I could work around it but moving a file there but I propose a new heuristic that solves the problem by finding the longest common path in the input files directories. Implementation inspired by Python's os.path.commonpath().

This is obviously doing a bit more work than before but I don't think it should cause a performance problem – most of the added work is sorting the directories, and I only sort them by string length.

@micampe
Copy link
Contributor Author

micampe commented Oct 7, 2023

Thinking about it as a more general solution, considering that the files could be anywhere, there could be no common path at all, which would return / or /Users/username, and doesn't make sense.

@helje5
Copy link
Member

helje5 commented Oct 7, 2023

Since I don't have any files in my target's root directory (only other subdirectories), the current heuristic can't find my sql file, because it only looks at those subdirectories

I think the subdirectories shouldn't matter? The inputFiles should contain ALL files that are part of the target, regardless where they live, even in subfolders?

Like say:

Project/
  Target/
    Subdir/
      A.swift
      Further/
        B.swift

It looks at "Subdir" and "Further" for "/Target", neither matches. So I think the modification needs to check the dirs whether they contain Target at a higher level, and then maybe use the longest path.

I.e. instead of just doing hasSuffix("/Target"), so .contains("/Target/") as a fallback.

I think the proposed patch is incorrect, as it would just always pick the longest subdir, regardless of whether it contains the name we are looking for. I.e. it fails this:

Project/
  Target/
    A.swift
    Subdir/
      Further/
        B.swift

which currently works as expected

@helje5 helje5 added the enhancement New feature or request label Oct 7, 2023
@helje5
Copy link
Member

helje5 commented Oct 7, 2023

Do you want to give this another try, or should I give it a shot?

@micampe
Copy link
Contributor Author

micampe commented Oct 7, 2023

I agree the patch is incorrect, but your second example works: the directories array would contain

Project/Target
Project/Target/Subdir/Further

and since it looks for the longest path prefix (not just the longest path) it finds Project/Target.

It fails when the target contains a file that is outside of its root directory, for example if Outside.swift here is part of TargetB (by using an absolute path, for example), my patch returns Project, which is wrong.

Project/
  TargetA/
    Outside.swift
Project/
  TargetB/
    Inside.swift

I'll try again with your suggestion of searching the longest path containing /Target/.

@micampe
Copy link
Contributor Author

micampe commented Oct 7, 2023

never mind, still wrong :)

@micampe
Copy link
Contributor Author

micampe commented Oct 7, 2023

ok, I think it works now. I'm not too happy with how complicated it is but I couldn't find a way to simplify it. If you have a better idea feel free to take over.

I also tweaked some of the printed messages that would have helped me to diagnose my problem.

Use the longest path containing the target name
@micampe
Copy link
Contributor Author

micampe commented Oct 9, 2023

New patch that I think is clearer.

@helje5 helje5 merged commit af27dea into Lighter-swift:develop Oct 10, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants