Use llbuild's new content-exclusion-patterns node attribute to prevent it from descending into .build and .git directories
#5594
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm actually not sure how a custom scratch directory could be excluded in a way that wouldn't have side effects — at the llbuild level, the exclusion is for directory entries and contains just names (last path components), so if the scratch directory is |
|
@swift-ci please smoke test |
tomerd
approved these changes
Jun 16, 2022
|
Confirmed that this fixes the problem, when the latest llbuild is used, and with the test case from #5482. Surprisingly using |
…ent it from descending into `.build` and `.git` directories This requires the llbuild commit a1adaff2a405c3a74925e60356431050cb9a3a9d, but it does not cause problems if the llbuild being used doesn't yet support this property. Instead it will be ignored. This adds a dedicated unit test with a flat package. We should consider whether any customized scratch directory should also be included in the list. This is a little tricky since the exclusion patterns are applied to all subdirectory name, not just those at the top level. In testing, it doesn't actually seem to be required, which seems a little surprising. The fix in this commit is safe and good on its own, and we can consider adding customized scratch paths later once we've looked deeper into that issue. rdar://93982172
abertelrud
force-pushed
the
eng/93982172-flat-packages-can-cause-spurious-swift-compilation
branch
from
c987281 to
f51fc51
Compare
|
Found a way to reproduce the problem using a test fixture, so I'm adding an end-to-end test as well. |
abertelrud
changed the title
content-exclusion-patterns node attribute to prevent it from descending into .build and .git directoriescontent-exclusion-patterns node attribute to prevent it from descending into .build and .git directories
abertelrud
added
ready
|
@swift-ci please smoke test |
abertelrud
removed
the
ready
|
@tomerd I managed to find a way to repro and add a unit test here. Wanted to let you know since it's a chance since last you looked. |
neonichu
approved these changes
Jun 16, 2022
abertelrud
deleted the
eng/93982172-flat-packages-can-cause-spurious-swift-compilation
branch
abertelrud
added a commit
to abertelrud/swift-package-manager
that referenced
this pull request
Jun 17, 2022
…ent it from descending into `.build` and `.git` directories (swiftlang#5594) This requires the llbuild commit a1adaff2a405c3a74925e60356431050cb9a3a9d, but it does not cause problems if the llbuild being used doesn't yet support this property. Instead it will be ignored. This adds a dedicated unit test with a flat package. We should consider whether any customized scratch directory should also be included in the list. This is a little tricky since the exclusion patterns are applied to all subdirectory name, not just those at the top level. In testing, it doesn't actually seem to be required, which seems a little surprising. The fix in this commit is safe and good on its own, and we can consider adding customized scratch paths later once we've looked deeper into that issue. rdar://93982172 (cherry picked from commit fb032ac)
abertelrud
added a commit
that referenced
this pull request
Jun 17, 2022
…ent it from descending into `.build` and `.git` directories (#5594) (#5599) This requires the llbuild commit a1adaff2a405c3a74925e60356431050cb9a3a9d, but it does not cause problems if the llbuild being used doesn't yet support this property. Instead it will be ignored. This adds a dedicated unit test with a flat package. We should consider whether any customized scratch directory should also be included in the list. This is a little tricky since the exclusion patterns are applied to all subdirectory name, not just those at the top level. In testing, it doesn't actually seem to be required, which seems a little surprising. The fix in this commit is safe and good on its own, and we can consider adding customized scratch paths later once we've looked deeper into that issue. rdar://93982172 (cherry picked from commit fb032ac)
|
This seems to have fixed #5482 (#5482 (comment)). Can we add #5505 as a regression test, then close both #5482 and #5505? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This requires the llbuild commit a1adaff2a405c3a74925e60356431050cb9a3a9d, but it does not cause problems if the llbuild being used doesn't yet support this property. Instead it will be ignored.
Also added a dedicated test with a flat package.
We should consider whether any customized scratch directory should also be included in the list. This is a little tricky since the exclusion patterns are applied to all subdirectory name, not just those at the top level. Oddly, in testing I did not repro the problem if I was using a custom scratch directory, so this may not be a issue in practice.
Note that this fixes the problem with doing spurious rebuilds (of which the spurious JSON output is just a symptom) for the case of a flat package structure, where the root directory of the package is considered to be a source directory. We have also seen this output when switching toolchains, which seems to be a completely different problem with similar symptoms.
rdar://93982172