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

Recognize multiple sentinel files for determining the build root #8105

Merged

Conversation

@Eric-Arellano
Copy link
Contributor

commented Jul 24, 2019

Problem

We cannot have implicit runtime dependencies on files in the buildroot - they must all be declared explicitly via BUILD entries. #8066 fixed several of these issues, but not all.

However, we cannot explicitly depend on the file ./pants because it breaks V1 test execution. When running tests with V1, the folder in .pants.d will strip the prefix so src/python/pants -> pants. You cannot both have a directory named pants and a file named pants, so this causes most V1 tests to fail.

Solution

Use multiple sentinel files to establish the buildroot, such as BUILD_ROOT. This allows us to safely depend on a sentinel file with very few code changes.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

Blocked by #8063.

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:fix-test-buildroot-issue branch 2 times, most recently from 275f951 to 9eec85c Jul 24, 2019

@Eric-Arellano Eric-Arellano changed the title WIP: Fix remaining unit test issues with runtime dependency on buildroot Fix remaining unit test issues with runtime dependency on buildroot Jul 24, 2019

@Eric-Arellano Eric-Arellano marked this pull request as ready for review Jul 24, 2019

@stuhood
Copy link
Member

left a comment

Thanks!

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

Challenge to landing this...depending on //:pants_script results in V1 breaking. Why? In V1, the .pants.d folder strips the source root from files, so the paths look like 13013/pants/util/strutil.py. We cannot have a file named pants at the same time that we have a folder named pants.

Pretty sure it's impossible to conditionally depend on //:pants_script in the BUILD, so instead we need to find some way to stop depending on //:pants_script in these tests.

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:fix-test-buildroot-issue branch from 83b4396 to 83bcda1 Jul 26, 2019

@Eric-Arellano Eric-Arellano requested review from stuhood and blorente Jul 26, 2019

@stuhood
Copy link
Member

left a comment

Looks like this is mostly working, so won't block. But I think an API to explicitly set the sentinel value might be cleaner.

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:fix-test-buildroot-issue branch 3 times, most recently from 7cf30c9 to d55e04a Jul 27, 2019

src/python/pants/base/build_environment.py Outdated Show resolved Hide resolved

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:fix-test-buildroot-issue branch 2 times, most recently from f796562 to e9de29f Jul 27, 2019

@Eric-Arellano Eric-Arellano requested a review from stuhood Jul 27, 2019

@stuhood
Copy link
Member

left a comment

Thanks!

BUILD Show resolved Hide resolved

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:fix-test-buildroot-issue branch from 8661809 to 1977755 Jul 27, 2019

@Eric-Arellano Eric-Arellano changed the title Fix remaining unit test issues with runtime dependency on buildroot Recognize `BUILD_ROOT` to determine the build root to fix V2 remote test failures Jul 27, 2019

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:fix-test-buildroot-issue branch from 1977755 to 0d2cea9 Jul 27, 2019

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:fix-test-buildroot-issue branch from 0d2cea9 to 3c3cb9e Aug 11, 2019

# (e.g., a JDK that is no longer present), or points to the wrong JDK.
if not jdk_home_symlink.exists() or jdk_home_symlink.resolve() != Path(underlying_dist.home):
safe_delete(str(jdk_home_symlink)) # Safe-delete, in case it's a broken symlink.
safe_mkdir_for(jdk_home_symlink)

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 11, 2019

Author Contributor

This line was necessary to get ./pants --no-v1 --v2 test tests/python/pants_test/backend/jvm/tasks/jvm_compile (note this is local execution).

When you include //:build_root in TestBase, if you were to run Path.cwd() at this particular line then it no longer results in the actual build root of /Users/eric/DocsLocal/code/projects/pants but instead the /Users/eric/DocsLocal/code/projects/pants/.pants.d/process-executionYmgTGM folder used by V2, which is what we want. As a result, it tries to create the symlink in .pants.d/process-executionYmgTGM/.pants.d, which does not exist. We add this line to ensure that the parent folder does exist, so that the symlink can be created properly.

@Eric-Arellano Eric-Arellano requested review from jsirois and stuhood Aug 11, 2019

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:fix-test-buildroot-issue branch from 3c3cb9e to b34e6f9 Aug 12, 2019

@Eric-Arellano Eric-Arellano changed the title Recognize `BUILD_ROOT` to determine the build root to fix V2 remote test failures Recognize multiple sentinel files for determining the build root to fix V2 remote test failures Aug 12, 2019

@Eric-Arellano Eric-Arellano changed the title Recognize multiple sentinel files for determining the build root to fix V2 remote test failures Recognize multiple sentinel files for determining the build root Aug 12, 2019

@blorente
Copy link
Contributor

left a comment

I don't have enough context to have an opinion on whether this is the right design, but the code looks correct :)

@Eric-Arellano Eric-Arellano merged commit 6695a69 into pantsbuild:master Aug 13, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:fix-test-buildroot-issue branch Aug 13, 2019

@stuhood stuhood referenced this pull request Aug 13, 2019
stuhood added a commit that referenced this pull request Aug 24, 2019
Recognize multiple sentinel files for determining the build root (#8105)
We cannot have implicit runtime dependencies on files in the buildroot - they must all be declared explicitly via `BUILD` entries. #8066 fixed several of these issues, but not all.

However, we cannot explicitly depend on the file `./pants` because it breaks V1 test execution. When running tests with V1, the folder in `.pants.d` will strip the prefix so `src/python/pants` -> `pants`. You cannot both have a directory named `pants` and a file named `pants`, so this causes most V1 tests to fail.

Use multiple sentinel files to establish the buildroot, such as `BUILD_ROOT`. This allows us to safely depend on a sentinel file with very few code changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.