Recognize multiple sentinel files for determining the build root #8105

Eric-Arellano · 2019-07-24T02:48:09Z

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 · 2019-07-24T05:17:59Z

Blocked by #8063.

stuhood

Thanks!

Eric-Arellano · 2019-07-26T03:02:23Z

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.

stuhood

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

src/python/pants/base/build_environment.py

stuhood

Thanks!

BUILD

Eric-Arellano · 2019-08-11T20:40:14Z

src/python/pants/backend/jvm/subsystems/zinc.py

+      # (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 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.

# Delete this line to force a full CI run for documentation-only changes. [ci skip] # Documentation-only change. # Delete this line to force a full CI run for documentation-only changes. [ci skip] # Documentation-only change.

blorente

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

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.

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

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 requested review from blorente, stuhood and illicitonion July 24, 2019 16:16

Eric-Arellano marked this pull request as ready for review July 24, 2019 21:41

stuhood approved these changes Jul 24, 2019

View reviewed changes

blorente approved these changes Jul 25, 2019

View reviewed changes

Eric-Arellano force-pushed the fix-test-buildroot-issue branch from 83b4396 to 83bcda1 Compare July 26, 2019 03:46

Eric-Arellano requested review from stuhood and blorente July 26, 2019 15:41

stuhood approved these changes Jul 26, 2019

View reviewed changes

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

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

stuhood approved these changes Jul 27, 2019

View reviewed changes

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

Eric-Arellano force-pushed the fix-test-buildroot-issue branch 2 times, most recently from f796562 to e9de29f Compare July 27, 2019 16:24

Eric-Arellano requested a review from stuhood July 27, 2019 16:27

stuhood approved these changes Jul 27, 2019

View reviewed changes

BUILD Show resolved Hide resolved

Eric-Arellano force-pushed the fix-test-buildroot-issue branch from 8661809 to 1977755 Compare July 27, 2019 23:39

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 force-pushed the fix-test-buildroot-issue branch from 1977755 to 0d2cea9 Compare July 27, 2019 23:46

Eric-Arellano force-pushed the fix-test-buildroot-issue branch from 0d2cea9 to 3c3cb9e Compare August 11, 2019 20:34

Eric-Arellano commented Aug 11, 2019

View reviewed changes

Eric-Arellano requested review from jsirois and stuhood August 11, 2019 23:34

Eric-Arellano added 12 commits August 11, 2019 17:46

Fix issues with config.py

9369b7b

Fix test_build_root.py (fails until pantsbuild#8063)

822586e

Fix fakes.py failure

f391dec

Remove passing targets from blacklist

648839b

No longer depend on ./pants

bae1eec

Rewrite new sentinel file mechanism to use property

c6e6087

Fix get_default_pants_config_file failing

7bdc657

Revert propogation of sentinel_filename args

3337713

Redesign solution: expand list of acceptable sentinel files

33adbb2

Update docs

57aa680

# Delete this line to force a full CI run for documentation-only changes. [ci skip] # Documentation-only change. # Delete this line to force a full CI run for documentation-only changes. [ci skip] # Documentation-only change.

Refactor zinc.py for better readability of the error

9d29d10

Fix Zinc failing due to missing .pants.d folder

b34e6f9

Eric-Arellano force-pushed the fix-test-buildroot-issue branch from 3c3cb9e to b34e6f9 Compare August 12, 2019 00:47

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 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 approved these changes Aug 12, 2019

View reviewed changes

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

Eric-Arellano deleted the fix-test-buildroot-issue branch August 13, 2019 00:19

stuhood mentioned this pull request Aug 13, 2019

Prepare 1.19.0rc1 #8169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recognize multiple sentinel files for determining the build root #8105

Recognize multiple sentinel files for determining the build root #8105

Eric-Arellano commented Jul 24, 2019 •

edited

Loading

Eric-Arellano commented Jul 24, 2019

stuhood left a comment

Eric-Arellano commented Jul 26, 2019

stuhood left a comment

stuhood left a comment

Eric-Arellano Aug 11, 2019

blorente left a comment

Recognize multiple sentinel files for determining the build root #8105

Recognize multiple sentinel files for determining the build root #8105

Conversation

Eric-Arellano commented Jul 24, 2019 • edited Loading

Problem

Solution

Eric-Arellano commented Jul 24, 2019

stuhood left a comment

Choose a reason for hiding this comment

Eric-Arellano commented Jul 26, 2019

stuhood left a comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Eric-Arellano Aug 11, 2019

Choose a reason for hiding this comment

blorente left a comment

Choose a reason for hiding this comment

Eric-Arellano commented Jul 24, 2019 •

edited

Loading