-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add objfw dependency #13067
Add objfw dependency #13067
Conversation
8a7dec1
to
46e553f
Compare
The Windows check failures do seem to be relevant, I think they're related to the changing the order of the compilers, since it looks like before that the test skipped on windows. |
dfb23dc
to
ed3ba44
Compare
That should be addressed now. It seems Meson expects MSVC-syntax from Clang on Windows, so it needs to be clang-cl instead. |
This needs documentation |
f7ef5f8
to
4e0cfb3
Compare
Done. |
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 few small nits, otherwise this looks good to me to land
endif | ||
|
||
executable('SimpleTest', 'SimpleTest.m', | ||
dependencies: [objfw_dep]) |
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.
Please merge the new 5 and 6 tests into a single test. Meson startup overhead is the longest pull in CI, and so we've moved to have fewer, but more complex tests rather than many simple ones.
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.
ObjFWTest is very new it looks like, which means that it's very likely for people with the framework installed to not have this and end up with the whole test being skipped.
Is it possible to use a different module that has been around for longer to do the test that modules:
works?
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.
@dcbaker Done.
@eli-schwartz Unfortunately, ObjFWTest is the first module ObjFW ships with. All others are 3rd party.
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.
[5/5] clang -o exe1 exe1.p/main.c.o -Wl,--as-needed -Wl,--no-undefined subprojects/cmTest/libcmTest.a
FAILED: exe1
clang -o exe1 exe1.p/main.c.o -Wl,--as-needed -Wl,--no-undefined subprojects/cmTest/libcmTest.a
/usr/bin/ld: subprojects/cmTest/libcmTest.a.p/cmTest.c.o: relocation R_X86_64_32 against `.rodata' can not be used when making a PIE object; recompile with -fPIE
/usr/bin/ld: failed to set dynamic section sizes: bad value
clang-15.0: error: linker command failed with exit code 1 (use -v to see invocation)
This looks related. The OpenSUSE runner is building C with cc
and ObjC with clang, then linking with clang. For some reason it doesn't like that but the reason it doesn't like it is because of PIE??
endif | ||
|
||
executable('SimpleTest', 'SimpleTest.m', | ||
dependencies: [objfw_dep]) |
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.
ObjFWTest is very new it looks like, which means that it's very likely for people with the framework installed to not have this and end up with the whole test being skipped.
Is it possible to use a different module that has been around for longer to do the test that modules:
works?
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.
Could we install objfw on:
- macOS CI via brew install in the GitHub actions yaml
- fedora via ci/ciimages/ in the list of packages
The latter will only start testing once the PR is merged and the images are rebuilt. The skip will function until then (we can check that it passes in Fedora since some image builders might fail, but the Fedora image builder should show objfw running successfully and should be building fine right now).
That does look to me like a very OpenSUSE-specific error where they probably force them flags on the linker. I'm guessing this is yet another case of "No proper separation between compilers / languages in Meson", so I'm not sure what to do about it. |
Added a commit. |
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.
All of my concerns have been addressed, so when @eli-schwartz is happy with this it can be merged
I looked into this and this is a distribution bug: OpenSUSE uses incompatible default flags for GCC and Clang: Their GCC defaults to not using PIE while their clang does. This leaves 2 options: 1.) Ignore the failure as a distribution bug (which it is!) You can easily reproduce this yourself by creating a file called #include <stdio.h>
int main() { puts("Hello world!"); return 0; } And then compiling like this:
Which will fail. However, this works:
As does this:
In any case, this problem exists irregardless of this PR, it just makes it more visible. |
Seems like we should skip your test on opensuse |
It's not even my test that's failing, it's the existing |
cf72dd8
to
bdb74f6
Compare
Worked around this by always enabling PIE in the test. |
We should get you to rebase on #13133 when it is merged that way the Fedora image builder will succeed. Also, it sounds like if you rebase on master, the Gentoo one will succeed too? |
Rebased on master for now |
This is good to go pending whether or not we want to wait my PR to merge to rebase on so the Fedora image builder job succeeds. |
89f79d0
to
355b4b5
Compare
Rebased for the 3rd time now - what's missing to get this merged? |
Our development rules prohibit merging MRs that break CI. |
The problem is that this PR doesn't break things, but it's things already being broken, and every time I rebase, something else has been broken. I guess then that this means this can never get merged because CI is always broken. |
I restarted failed jobs, sadly there is some flakiness in some tests. :( But for example #13148 has all green CI (except for the lint one which is an actual failure) and that is only one day old. |
I'm just waiting for the full CI results to come in... As I pointed out a couple times in the past already -- the development policy is that the CI must be green in order to merge the PR, but "CI image builder" jobs are exempt. You don't need to worry about them, you don't need to rebase to try fixing it, and when I'm ready to merge this PR I will manually check that all required CI jobs pass and that all failures are "just" image builder failures. At the moment we are fine and this is on track to be merged as long as there are no additional failures. All three red jobs are image builder jobs. |
Ok, interesting. 3 hours ago, this passed: https://github.com/mesonbuild/meson/actions/runs/8852155560/job/24310221953 Now it fails in this PR:
It's mixing
We are definitely using the latter. The former not, but maybe that is a mistaken diagnosis from using different toolchains? Linking bitcode bundles together is hardly likely to work without a consistent toolchain. The key insight is here:
We are trying to use clang in preference to gcc, but what we ended up doing is using clang in preference to the system AppleClang. |
That's exactly what I mean. This error only came up after rebasing. Every time I rebase to incorporate one fix for CI, it also incorporates something that breaks another test on CI. |
GCC only has very limited support for Objective-C and doesn't support any of the modern features, so whenever Clang is available, it should be used instead. Essentially, the only reason to ever use GCC for Objective-C is that Clang simply does not support the target system.
This uses objfw-config to get to the flags, however, there's still several todos that can only be addressed once dependencies can have per-language flags.
91b0e85
to
e1afd17
Compare
OBJC=clang / OBJCXX=clang++ can pick up a newer Clang that no longer supports bitcode.
Everything but image builders seems to be back to green now -> I'm not rebasing it anymore |
Thanks for persevering. :) |
No description provided.