-
Notifications
You must be signed in to change notification settings - Fork 168
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
Stop forcing static LibXML2 build on Linux #1601
Conversation
Do force static linking when building the Linux wheel Document the added environment variables
# CIBW_BEFORE_ALL_LINUX: "yum install -y libuuid-devel && yum install -y libxml2-devel" | ||
# CIBW_ENVIRONMENT_LINUX: "CFLAGS='-I/usr/include/libxml2'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question: are we keeping this uncommented so users can force a static installation if needed, or for our own references in the future? If we're going to keep it as a comment, could we add a sentence here or in the docs what these commented lines do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first thing I would say is that the github workflows are pretty much entirely "ours" -- the contents of these files have no impact on "downstream" users, except perhaps in the sense that users who clone the repo on github itself might activate workflows (but probably not this one, since I don't think we trigger it ourselves except at release time).
I suppose I kept the commented lines as a reference for how to build a wheel on "GitHub-world" Linux dynamically linked to LibXML2. You tracked that info down at some point, and even if we've decided it doesn't work it may be useful down the road.
As for comments my view is that because the lines below include setting FORCE_BUILD_LIBXML2
it's pretty clear what's going on, but YMMV.
@@ -5,7 +5,7 @@ | |||
select = E,F,W,B,B901,B902,B903 | |||
|
|||
# ignore W504 line break before binary operator | |||
ignore = W504 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed a new flake8 warning and adding it to ignore is fine for now but reading it I think we should probably make some efforts to update the test code (per the flake8 warning: be more specific than pytest.raises(Exception)
– use the specific Exception that is expected to be raised) and remove B017 from the ignore list once the affected tests are updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I added this because it suddenly appeared and was causing the tests to fail. I'm fine with fixing it when we have time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me with your comments on my question + agreement on fixing flake8 errors once we have time in the future (but we should make a separate issue for that)
Checklist: