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

support versioned symbols with more underscores #73

Merged
merged 3 commits into from
May 24, 2017

Conversation

reaperhulk
Copy link
Contributor

Fixes #72. I'm unsure if this test is useful so let me know if I should be doing something different.

@@ -14,3 +16,5 @@ def test_policy_checks_glibc():
assert policy > POLICY_PRIORITY_LOWEST
policy = versioned_symbols_policy({"some_library.so": {"GLIBC_999"}})
assert policy == POLICY_PRIORITY_LOWEST
policy = versioned_symbols_policy({"some_library.so": {"OPENSSL_1_1_0"}})
assert policy == POLICY_PRIORITY_HIGHEST
Copy link
Member

Choose a reason for hiding this comment

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

Since `OPENSSL_1_1_01 isn't in the whitelist, I might have expect this to give POLICY_PRIORITY_LOWEST, but perhaps there's something I'm not thinking of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I don't understand the policy priorities so I have no idea if this assertion is correct!

Copy link
Member

Choose a reason for hiding this comment

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

@rmcgibbo: So the policy whitelist only applies to GCC*/CXX* symbol versions (see auditwheel/policy/policy.json for the full list). Any other symbol version will automatically satisfy the highest policy. If that wasn't the case, then no external dependency symbols (e.g. OPENSSL_1_1_0, KERBEROS_5_MIT) would satisfy the manylinux1 requirements.

@rmcgibbo
Copy link
Member

Does auditwheel with this patch repair your openssl-linked wheel properly?

@reaperhulk
Copy link
Contributor Author

auditwheel does properly repair my openssl-linked wheel with this patch applied.

Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

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

This looks good to me! Thanks for the fix; I didn't review the fix in #54 earlier, so I missed that it broke this type of symbol version check.

I am requesting one additional test, if that's alright.

@@ -24,7 +24,7 @@ def policy_is_satisfied(policy_name: str,
required_vers = {} # type: Dict[str, Set[str]]
for symbols in versioned_symbols.values():
for symbol in symbols:
sym_name, _ = symbol.split("_", 2)
sym_name, _ = symbol.split("_", 1)
Copy link
Member

Choose a reason for hiding this comment

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

Yay fixing off by 1 error.

@@ -14,3 +16,5 @@ def test_policy_checks_glibc():
assert policy > POLICY_PRIORITY_LOWEST
policy = versioned_symbols_policy({"some_library.so": {"GLIBC_999"}})
assert policy == POLICY_PRIORITY_LOWEST
policy = versioned_symbols_policy({"some_library.so": {"OPENSSL_1_1_0"}})
assert policy == POLICY_PRIORITY_HIGHEST
Copy link
Member

Choose a reason for hiding this comment

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

@rmcgibbo: So the policy whitelist only applies to GCC*/CXX* symbol versions (see auditwheel/policy/policy.json for the full list). Any other symbol version will automatically satisfy the highest policy. If that wasn't the case, then no external dependency symbols (e.g. OPENSSL_1_1_0, KERBEROS_5_MIT) would satisfy the manylinux1 requirements.

@@ -14,3 +16,5 @@ def test_policy_checks_glibc():
assert policy > POLICY_PRIORITY_LOWEST
policy = versioned_symbols_policy({"some_library.so": {"GLIBC_999"}})
assert policy == POLICY_PRIORITY_LOWEST
policy = versioned_symbols_policy({"some_library.so": {"OPENSSL_1_1_0"}})
assert policy == POLICY_PRIORITY_HIGHEST
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind adding one more test asserting SYMBOLWITHOUTUNDERSCORES satisfies POLICY_PRIORITY_HIGHEST or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will in fact fail since the assignment assumes 2 values. Is it possible for a symbol to be entirely unversioned? I guess the answer to that is likely to be yes?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, symbols can be unversioned. The symbol version spec is very handwavey. We should probably change the way we're handling that split such that we only get the first value it returns (rather than trying to do a tuple unpack).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use partition here to do this. I'll swap it.

@njsmith njsmith mentioned this pull request May 23, 2017
@ehashman ehashman merged commit 3163c09 into pypa:master May 24, 2017
@ehashman
Copy link
Member

@rmcgibbo Do you mind cutting a release for this? I would if I had access :)

@matthew-brett
Copy link
Contributor

@ehashman - I guess you mean pypi access? Sounds like a good idea to me. Thanks for all the work so far.

@ehashman
Copy link
Member

Yeah, tagging and uploading a release to PyPI. I don't know what the process is so I figured I'd ask.

@matthew-brett
Copy link
Contributor

I think you'll need @rmcgibbo or @ogrisel to give you maintainer permissions on pypi, in order to upload.

@rmcgibbo - would you mind throwing a few notes about release procedure down, either here or in a text file somewhere in the distribution?

@rmcgibbo
Copy link
Member

@ehashman: what's you username on pypi.org?

@ehashman
Copy link
Member

I am also ehashman there.

@rmcgibbo
Copy link
Member

The release procedure I've used so far is pretty simple.

  1. The project's setup.py/setup.cfg uses pbr, so to increment the version number, you just make a new git tag. So something like git tag -s 1.6 -m 'Release version 1.6' I think, and git push origin --tags`.
  2. And then I've been uploading sdists and wheels to PyPI (python setup.py sdist upload && python setup.py bdist_wheel upload).

@rmcgibbo
Copy link
Member

@ehashman: great. you should be listed as an auditwheel maintainer on pypi.org now.

@ehashman
Copy link
Member

Released.

@matthew-brett
Copy link
Contributor

Excellent - thanks.

@njsmith
Copy link
Member

njsmith commented May 26, 2017

And I just submitted the PR to rev the manylinux docker images: pypa/manylinux#116

@ehashman: just so you know what's going on here – the manylinux docker images are always install the latest auditwheel from PyPI when they're re-built, but they only get re-built when a commit lands in the pypa/manylinux master branch (which happens via a magic travis job that pushes the new images to quay.io). Generally we follow the one-person-submits-PR, different-person-hits-merge model. As a member of the "manylinux team" you should have write access to the manylinux repo, so you could hit merge, or in the future if you do another auditwheel release it might make sense to submit a PR like this as part of the release checklist.

It suddenly occurs to me that we might be able to trick requires.io into submitting these PRs for us, even though it doesn't really fit their normal model...

@reaperhulk
Copy link
Contributor Author

@njsmith Travis has a beta feature where you can set up scheduled builds now too, so it might make sense just to have it rebuild and push to quay every day? If nothing has changed then all the layers will be identical and it will essentially be a noop.

@njsmith
Copy link
Member

njsmith commented May 26, 2017

@reaperhulk: Interesting point. I suppose a 0-24 hour delay in getting new auditwheels into the docker image wouldn't be a big deal... in theory it's nice to be quick, but it's not like we've been reliably quick with the current system either! Nightly builds would also start failing immediately as soon as one of our dependencies breaks (e.g. openssl releases a new version and moves the old one, or when centos5 went EOL and they moved the repo). I'm not 100% sure whether this is an advantage or disadvantage :-). I suppose this wouldn't hurt anything, just might mean annoying email for whoever is configured to get those. Maybe we wouldn't even notice at all :-).

@reaperhulk reaperhulk deleted the fix-72 branch May 26, 2017 22:51
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.

5 participants