Mark `pants` as an explicit namespace package (rather than implicit) #17563

thejcannon · 2022-11-17T16:59:01Z

Now we aren't touching our source tree during tests, which is causing unintended side-effects in #17520, because really we shouldn't be assuming the process sandboxes for tests are mutable (at least in the source tree). Not to mention that lengthy comment in conftest.py which should raise alarm.

stuhood

Thanks!

You'll want to force this to build/test the wheels, to make sure it doesn't break anything in there.

src/python/pants/__init__.py

thejcannon · 2022-11-17T17:42:19Z

(temporarily touching pants.toml to force CI to build+test wheels)

kaos · 2022-11-17T17:45:34Z

(temporarily touching pants.toml to force CI to build+test wheels)

coolest touch I've seen! :D

benjyw

That comment is precisely what I wanted, thanks!

Eric-Arellano · 2022-11-17T18:11:13Z

pants.toml

+#            ___________    ____
+#     ______/   \__//   \__/____\
+#   _/   \_/  :           //____\\
+#  /|      :  :  ..      /        \
+# | |     ::     ::      \        /
+# | |     :|     ||     \ \______/
+# | |     ||     ||      |\  /  |
+#  \|     ||     ||      |   / | \
+#   |     ||     ||      |  / /_\ \
+#   | ___ || ___ ||      | /  /    \
+#    \_-_/  \_-_/ | ____ |/__/      \
+#                 _\_--_/    \      /
+#                /____             /
+#               /     \           /
+#               \______\_________/
+#


huh? nevermind

(temporarily touching pants.toml to force CI to build+test wheels)

huh? nevermind

was my reaction too... but I was "fortunate" enough to spot Josh's comment before saying anything ;)

thejcannon · 2022-11-17T18:28:04Z

subprocess.CalledProcessError: Command '[PosixPath('/var/folders/y3/3xtfkj390kg4w2v44gyyfl7h0000gp/T/tmpf6fv4e0x/bin/python'), '-c', 'import pants.testutil.option_util, pants.testutil.rule_runner, pants.testutil.pants_integration_test']' returned non-zero exit status 1.

ruh roh

thejcannon · 2022-11-17T18:46:44Z

Oh so the failures are only on MacOS? Bother...

build-support/bin/_release_helper.py

benjyw · 2022-11-18T16:18:18Z

What's the issue with VERSION? That has been a constant thorn in our side.

thejcannon · 2022-11-18T16:21:12Z

@benjyw I updated the description to explain VERSION issues

thejcannon · 2022-11-18T16:29:50Z

Buddy PR: pantsbuild/setup#135

chrisjrn · 2022-11-18T16:37:02Z

Buddy PR: pantsbuild/setup#135

Suggest not merging this PR until we are confident that we can update the launcher script to be useful for people who are using PANTS_FROM_SHA. See the linked ticket for discussion.

src/python/pants/version/BUILD

jsirois

I missed the VERSION shenanigans. Its not clear to me this is ship-itable. I'll bow out - the overall motivation seems weakish and this VERSION bit seems slapdashish.

benjyw

Not a fan of code in __init__.py, and we don't do that anywhere else in the repo, so why here? I'm not sure it's important that the import name remain the same? This isn't part of an external API...

Also, possibly we can temporarily have a copy of VERSION in the root, temporarily, until we sort out the pants script issues?

thejcannon · 2022-11-18T16:48:19Z

Not a fan of code in init.py, and we don't do that anywhere else in the repo, so why here? I'm not sure it's important that the import name remain the same? This isn't part of an external API...

On the contrary, the module name is version and not _version or anything likewise "private". And to add fuel to the fire, with the rate at which we break the plugin API I would completely expect plugins in the wild to change their declarations based on Pants version. I wouldn't feel comfortable changing this in a non-backwards-compatible way.

An alternative solution is to keep version.py and have it re-export symbols from pants._version.<whatever>.

thejcannon · 2022-11-18T16:52:47Z

@jsirois Can you be more specific? Your message only tells me how you feel about the change (and it isn't great) and not how to improve it so you feel differently. Otherwise it feels almost like criticism without the "constructive" bit 😞

jsirois · 2022-11-18T16:56:09Z

The constructive bit is I'm clueless what is motivating this change. FWICT this shifts things around with no clear purpose or real upside. It started as that with the NS package re-jiggle and then added more apparently unmotivated changes with the VERSION thing. As such there is too much I don't understand here - clearly - to be a useful reiewer.

thejcannon · 2022-11-18T16:57:21Z

Hm, perhaps I split the VERSION change off into a dedicated PR then.

Hold on tight! 😄

(also, next time you're very welcome to say "hey I dont' understand why we're doing this. Can you update the description and/or make smaller PRs?". It's a lot less disheartening, and I very much value feedback. Especially yours)

jsirois · 2022-11-18T16:58:50Z

@thejcannon thanks. I'll stay bowed out of both though. It seems like others already active here may be clued in more.

thejcannon · 2022-11-18T17:13:14Z

OK version moving is now in #17582.

This moves `VERSION` to be in a subdirectory of the main `pants` package. This is because loading resources from top-level names of namespace packages is extremely hand-wavy and ambiguous (Who "owns" `pants`? The `pants` package or the `pants.testutil` package?) So now we have it unambiguously in `pants/_version/`. This pain is seen in #17563 which tries to convert `pants` to be an _explicit_ namespace package, which truly messes up resource loading via `pkgutil` and `importlib.resources` ([this long ticket](python/importlib_resources#68) has some context, too). This PR makes `_version/VERSION` a symlink to the existing `VERSION` so that https://github.com/pantsbuild/setup isn't broken. This "works" because Pants is symlink oblivious in source-tree traversal, and therefore sees `pants/_version/VERSION` as `pants/VERSION`.

thejcannon · 2022-11-21T21:47:13Z

Wheel building passed.

thejcannon added 3 commits November 17, 2022 10:53

Make it explicit

ace5474

more comment

da89e22

woohoo

6f2aea3

thejcannon added the category:internal CI, fixes for not-yet-released features, etc. label Nov 17, 2022

thejcannon requested review from stuhood, benjyw and chrisjrn November 17, 2022 16:59

Eric-Arellano requested a review from jsirois November 17, 2022 17:00

stuhood approved these changes Nov 17, 2022

View reviewed changes

kaos approved these changes Nov 17, 2022

View reviewed changes

benjyw reviewed Nov 17, 2022

View reviewed changes

src/python/pants/__init__.py Outdated Show resolved Hide resolved

thejcannon added 2 commits November 17, 2022 11:17

moredocs

8ed4950

touch

b381af1

benjyw approved these changes Nov 17, 2022

View reviewed changes

Eric-Arellano reviewed Nov 17, 2022

View reviewed changes

Move VERSION

128fb70

benjyw reviewed Nov 18, 2022

View reviewed changes

build-support/bin/_release_helper.py Outdated Show resolved Hide resolved

cleanup

0633e8e

chrisjrn mentioned this pull request Nov 18, 2022

Pants should be able to tell if its launcher script is up-to-date #17581

Open

thejcannon mentioned this pull request Nov 18, 2022

Update shim for new VERSION location pantsbuild/setup#135

Closed

thejcannon added the needs-cherrypick label Nov 18, 2022

thejcannon added this to the 2.13.x milestone Nov 18, 2022

jsirois approved these changes Nov 18, 2022

View reviewed changes

src/python/pants/version/BUILD Outdated Show resolved Hide resolved

jsirois reviewed Nov 18, 2022

View reviewed changes

blank

7e66524

benjyw reviewed Nov 18, 2022

View reviewed changes

thejcannon mentioned this pull request Nov 18, 2022

Move version info/handling to a subdirectory #17582

Merged

undo VERSION changes

a519848

thejcannon added 2 commits November 21, 2022 13:23

Merge branch 'main' into explicit-namespace

450fdee

remove bump, which oddly will bump

1312abd

thejcannon mentioned this pull request Nov 21, 2022

Remove finger-point in pants.toml #17607

Closed

thejcannon merged commit a405486 into pantsbuild:main Nov 21, 2022

thejcannon deleted the explicit-namespace branch November 21, 2022 23:29

jsirois mentioned this pull request Nov 26, 2022

Prepare the 2.16.0.dev1 release. #17648

Merged

thejcannon removed the needs-cherrypick label Jan 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark `pants` as an explicit namespace package (rather than implicit) #17563

Mark `pants` as an explicit namespace package (rather than implicit) #17563

thejcannon commented Nov 17, 2022 •

edited

Loading

stuhood left a comment

thejcannon commented Nov 17, 2022

kaos commented Nov 17, 2022

benjyw left a comment

Eric-Arellano Nov 17, 2022 •

edited

Loading

thejcannon Nov 17, 2022

kaos Nov 17, 2022

thejcannon commented Nov 17, 2022

thejcannon commented Nov 17, 2022

benjyw commented Nov 18, 2022

thejcannon commented Nov 18, 2022 •

edited

Loading

thejcannon commented Nov 18, 2022

chrisjrn commented Nov 18, 2022

jsirois left a comment

benjyw left a comment

thejcannon commented Nov 18, 2022

thejcannon commented Nov 18, 2022

jsirois commented Nov 18, 2022

thejcannon commented Nov 18, 2022 •

edited

Loading

jsirois commented Nov 18, 2022

thejcannon commented Nov 18, 2022

thejcannon commented Nov 21, 2022

Mark pants as an explicit namespace package (rather than implicit) #17563

Mark pants as an explicit namespace package (rather than implicit) #17563

Conversation

thejcannon commented Nov 17, 2022 • edited Loading

stuhood left a comment

Choose a reason for hiding this comment

thejcannon commented Nov 17, 2022

kaos commented Nov 17, 2022

benjyw left a comment

Choose a reason for hiding this comment

Eric-Arellano Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

thejcannon Nov 17, 2022

Choose a reason for hiding this comment

kaos Nov 17, 2022

Choose a reason for hiding this comment

thejcannon commented Nov 17, 2022

thejcannon commented Nov 17, 2022

benjyw commented Nov 18, 2022

thejcannon commented Nov 18, 2022 • edited Loading

thejcannon commented Nov 18, 2022

chrisjrn commented Nov 18, 2022

jsirois left a comment

Choose a reason for hiding this comment

benjyw left a comment

Choose a reason for hiding this comment

thejcannon commented Nov 18, 2022

thejcannon commented Nov 18, 2022

jsirois commented Nov 18, 2022

thejcannon commented Nov 18, 2022 • edited Loading

jsirois commented Nov 18, 2022

thejcannon commented Nov 18, 2022

thejcannon commented Nov 21, 2022

Mark `pants` as an explicit namespace package (rather than implicit) #17563

Mark `pants` as an explicit namespace package (rather than implicit) #17563

thejcannon commented Nov 17, 2022 •

edited

Loading

Eric-Arellano Nov 17, 2022 •

edited

Loading

thejcannon commented Nov 18, 2022 •

edited

Loading

thejcannon commented Nov 18, 2022 •

edited

Loading