-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
test cases/linuxlike/14 static dynamic linkage: Add matrix by static #13842
base: master
Are you sure you want to change the base?
Conversation
https://github.com/mesonbuild/meson/actions/runs/11594506125/job/32280901101#step:5:587
Is it my fault? |
During "cygwin / gccx64ninja (pull_request) Failing", with the help of debug print, I've found: For dynamic linkage (static=false), nm output in "linuxlike / static dynamic linkage" filtered by "zlibVersion" is:
For static, it doesn't contain the symbol with "_imp". |
78a74fd
to
b7f1d3c
Compare
Small nit: commit messages should use imperative mood: https://cbea.ms/git-commit/#imperative |
b7f1d3c
to
629e79f
Compare
@eli-schwartz Done! Thanks! |
629e79f
to
a509016
Compare
else | ||
has_static = true | ||
endif | ||
project('zlib linkage', 'c') |
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 project description doesn't really need to change here (it is still testing static dynamic linkage, not zlib specifically, anyway :))
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.
Shall be fixed! Thanks.
# Solaris does not ship static libraries | ||
if host_machine.system() == 'sunos' |
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.
We should probably still explicitly skip on Solaris. Your new approach would cause test failure if SKIP_STATIC_ZLIB is not exported, even though we know that if s and host_machine.system() == 'sunos'
there will not be a static library.
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.
You are perfectly right that this changes the behavior, and it's better to keep the previous logic if we don't want to break something. Nice catch!
I will do what you are asking, but let's go a little farther...
When my new approach cause test failure
As far as I know, there are no SunOS tests in the current CI. And, we both know, the green status is showing this.
So there should be some test cases outside the project, the dark matter, where people will have this issue.
When my new approach fixes problem where it could possibly cause test failure
AFAIK, SunOS could be configured to have the static zlib version in addition to the default dynamic one. In this case, keeping the old condition is a bug.
Let's ask the author of those lines @alanc
With my approach the user of this test case will be able to change his environment to adopt the usage on SunOS for his/her setup.
Symmetry / Special case
What is so special about SunOS to include it into this build script?
What about the default configuration on MS Windows (no Cygwin) or MacOS?
What about IBM i (AS/400) and BSD families?
Gentoo setup is configured for Boost with the help of SKIP_STATIC_BOOST in the GHA yaml file. Why could not SunOS be configured in the same way?
Bottom line
As I said, I will fix this to keep this special case for SunOS, but IMHO it must go.
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.
While there is no SunOS (Solaris or illumos) instance in the CI, we run the test suite on Solaris when building the meson packages for Solaris. It is true that it is not impossible for a user to build and install a static version of libz on Solaris - but since we provide a dynamic library for libz as part of the OS packages, I don't know of anyone who bothers to build their own static version. I can't speak to other OS'es or why they didn't hit the problem - perhaps their default configuration doesn't have dynamic libz either instead of just dynamic and not static.
I'm not opposed to moving this from a line of code to a configuration file - what's the "GHA yaml file"?
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.
$ git --no-pager grep -C3 STATIC_BOOST
.github/workflows/cygwin.yml- env:
.github/workflows/cygwin.yml- # Cygwin's static boost installation is broken (some static library
.github/workflows/cygwin.yml- # variants such as boost_thread are not present)
.github/workflows/cygwin.yml: SKIP_STATIC_BOOST: 1
.github/workflows/cygwin.yml- shell: C:\cygwin\bin\bash.exe --noprofile --norc -o igncr -eo pipefail '{0}'
.github/workflows/cygwin.yml-
.github/workflows/cygwin.yml- - uses: actions/upload-artifact@v4
--
ci/ciimage/fedora/image.json- "base_image": "fedora:latest",
ci/ciimage/fedora/image.json- "env": {
ci/ciimage/fedora/image.json- "CI": "1",
ci/ciimage/fedora/image.json: "SKIP_STATIC_BOOST": "1",
ci/ciimage/fedora/image.json- "MESON_CI_JOBNAME": "linux-fedora-gcc"
ci/ciimage/fedora/image.json- }
ci/ciimage/fedora/image.json-}
--
ci/ciimage/gentoo/image.json- "env": {
ci/ciimage/gentoo/image.json- "CI": "1",
ci/ciimage/gentoo/image.json- "MESON_CI_JOBNAME": "linux-gentoo-gcc",
ci/ciimage/gentoo/image.json: "SKIP_STATIC_BOOST": "1"
ci/ciimage/gentoo/image.json- }
ci/ciimage/gentoo/image.json-}
--
ci/ciimage/opensuse/image.json- "base_image": "opensuse/tumbleweed:latest",
ci/ciimage/opensuse/image.json- "env": {
ci/ciimage/opensuse/image.json- "CI": "1",
ci/ciimage/opensuse/image.json: "SKIP_STATIC_BOOST": "1",
ci/ciimage/opensuse/image.json- "SINGLE_DUB_COMPILER": "1",
ci/ciimage/opensuse/image.json- "MESON_CI_JOBNAME": "linux-opensuse-gcc"
ci/ciimage/opensuse/image.json- }
--
test cases/frameworks/1 boost/test.json- "matrix": {
test cases/frameworks/1 boost/test.json- "options": {
test cases/frameworks/1 boost/test.json- "static": [
test cases/frameworks/1 boost/test.json: { "val": "true", "skip_on_env": [ "SKIP_STATIC_BOOST" ] },
test cases/frameworks/1 boost/test.json- { "val": "false" }
test cases/frameworks/1 boost/test.json- ],
test cases/frameworks/1 boost/test.json- "b_vscrt": [
For all our docker-based CI testing we generate the docker images themselves with /ci/env_vars.sh
as a kind of "/etc/profile for CI".
For Cygwin, which isn't docker-based, we use the GitHub Actions (GHA) yaml file .github/workflows/cygwin.yml
. We don't maintain any such thing for Solaris' packaging.
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.
Thank you, @alanc !
@eli-schwartz, should we throw away the SunOS special condition, or merge the PR in the current state?
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.
Excuse me, @eli-schwartz, what do you think?
a509016
to
edabf58
Compare
edabf58
to
9cec3d6
Compare
Not only Solaris doesn't ship static libraries, it's common practice for Linux distributions in general (security updates, size/space, etc.). See, for example 'test cases/frameworks/1 boost'. Enhancements: * Added flexibility: Introduced the SKIP_STATIC_ZLIB environment variable to optionally skip static linkage (similar to SKIP_STATIC_BOOST). * Expanded testing: Included testing for dynamic linkage, which was previously limited to static in verify_static.py (renamed to verify_zlib_linkage.py). * Improved test coverage: Ensured that each case is tested for its specific linkage type and the opposite case fails. * Addressed Cygwin issue: Ensured that the corresponding symbol is present on Cygwin. Renames, comments, etc.
9cec3d6
to
c9157c9
Compare
Only one fatal error not caused by my changes:
https://github.com/mesonbuild/meson/actions/runs/12135619615/job/33835043547?pr=13842 |
Not only Solaris doesn't ship static libraries, it's common practice for Linux distributions in general (security updates, size/space, etc.).
See, for example 'test cases/frameworks/1 boost'.
Enhancements:
Renames, comments, etc.