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

Process wheelhouse.txt holistically rather than per-layer #569

Merged
merged 5 commits into from
Jun 25, 2020

Conversation

johnsca
Copy link
Contributor

@johnsca johnsca commented Jun 23, 2020

Processing the wheelhouse.txt file from each layer in isolation means that pip can't resolve conflicts, duplicates, or dependency version pinning which spans across layers. This can lead to duplicate versions of libraries being installed which violate constraints to avoid certain upstream bugs.

This came up in both of:

And in both cases trying to resolve it on a per-layer or per-charm basis results in a whack-a-mole process of catching all of the layers which indirectly reintroduce the unpinned version of a
dependency pinned in a base layer.

johnsca added 5 commits June 23, 2020 09:52
Processing the wheelhouse.txt file from each layer in isolation means
that pip can't resolve conflicts, duplicates, or dependency version
pinning which spans across layers.  This can lead to duplicate versions
of libraries being installed which violate constraints to avoid certain
upstream bugs.

This came up in both of:

* https://bugs.launchpad.net/charm-flannel/+bug/1884598
* https://bugs.launchpad.net/charm-octavia/+bug/1884164

And in both cases trying to resolve it on a per-layer or per-charm basis
results in a whack-a-mole process of catching all of the layers which
indirectly reintroduce the unpinned version of a dependency pinned in a
base layer.
This allows higher layers to override explicit requirements while still
inheriting specs from lower layers.
@johnsca johnsca marked this pull request as ready for review June 23, 2020 20:57
@johnsca johnsca changed the title Process wheelhouse.txt wholistically rather than per-layer Process wheelhouse.txt holistically rather than per-layer Jun 23, 2020
@johnsca johnsca requested review from thedac and tvansteenburgh June 23, 2020 21:49
Copy link

@thedac thedac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this with Octavia. I am still getting two versions of netaddr:
netaddr-0.7.19.tar.gz netaddr-0.7.20.tar.gz

0.7.19 we want and is pinned in layer basic. I suspect neutron in the Octavia wheelhoust.txt is pulling in the newer version as many many packages depend on it and may or may not have constraints.

Copy link

@thedac thedac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction tests look good. Let's land this.

help="Show post-build report of changes")
parser.add_argument('-R', '--no-report', action="store_false",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-r and -R seem to be contradictory; how about we kill the -R codepath and just use -r=True|False instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to figure out a way to implement the -r[=true/|false] pattern but the case that always breaks is the current behavior that many may be using:

$ charm build -r ./src

There's no way with argparse to say "consume an arg, but only if it matches one of these choices". It will either not consume any args, always consume an arg, or try to consume an arg but blow up if it doesn't match one of the choices.

Additionally, I found at least one example with git -p and git -P which have the same contradictory meaning, and I know I've seen other cases and not found it terribly confusing. In general, and in the case of this change, the behavior is what I would expect: either of the options can be given, even multiple times, and the last one present on the command line takes precedence.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnsca i appreciate the attempt. I didn't intend for you to bend over backwards, but it sure would have been nice if "honor a bool value" would just work for argparse optargs. Anywho, I'm annoyed but not blocking contradictory flags; as you said, last in gets the cake, and i think that's a reasonable UX that charm builders can shoulder.

All that said, given that --report is now the default, I would ask you to consider #570. That is, deprecate -r with some stderr message that says -r is default and therefore deprecated; use -R to prevent charm build reports.

Copy link
Contributor

@kwmonroe kwmonroe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some additional commentary to be considered in #570.

@johnsca johnsca merged commit 695fd7a into master Jun 25, 2020
@johnsca johnsca deleted the johnsca/lp/1884598/wholistic-wheelhouse branch June 25, 2020 21:28
@johnsca
Copy link
Contributor Author

johnsca commented Jun 26, 2020

This has been released in the snap and to PyPI with 2.7.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants