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

Fix CI and make libcurl a hard-dependency when using libobjc2 #447

Merged
merged 22 commits into from
Oct 10, 2024
Merged

Conversation

hmelder
Copy link
Contributor

@hmelder hmelder commented Sep 8, 2024

The build system is really fragile.

if "${CC}" -v 2>&1 | grep -q 'clang version' does not work when the CC variable consists of more than one word, as it is treated as the executable.

Additionally, Win32 Native Threads was enabled on MinGW when running GCC which caused a build failure.

@hmelder hmelder requested a review from rfm as a code owner September 8, 2024 15:49
@hmelder
Copy link
Contributor Author

hmelder commented Sep 8, 2024

Seems like lld-link is once again discarding objc sections...

Making all for test_tool simpleTaskTests...
 Linking test_tool simpleTaskTests ...
lld-link: error: relocation against symbol in discarded section: __start_.objcrt$SEL
>>> referenced by ./obj/simpleTaskTests.obj/simpleTaskTests.m.o:(.objc_init)

lld-link: error: relocation against symbol in discarded section: __stop.objcrt$SEL
>>> referenced by ./obj/simpleTaskTests.obj/simpleTaskTests.m.o:(.objc_init)

lld-link: error: relocation against symbol in discarded section: __start_.objcrt$CLS
>>> referenced by ./obj/simpleTaskTests.obj/simpleTaskTests.m.o:(.objc_init)

lld-link: error: relocation against symbol in discarded section: __stop.objcrt$CLS
>>> referenced by ./obj/simpleTaskTests.obj/simpleTaskTests.m.o:(.objc_init)

lld-link: error: relocation against symbol in discarded section: __start_.objcrt$CLR
>>> referenced by ./obj/simpleTaskTests.obj/simpleTaskTests.m.o:(.objc_init)

lld-link: error: relocation against symbol in discarded section: __stop.objcrt$CLR
>>> referenced by ./obj/simpleTaskTests.obj/simpleTaskTests.m.o:(.objc_init)

@triplef
Copy link
Member

triplef commented Sep 9, 2024

@qmfrederik any idea why we're getting those linker errors now with MSVC and the MinGW errors?

@qmfrederik
Copy link
Contributor

@triplef The msvc job started failing when the NSURLSession rewrite (#422) PR got merged. This is the PR that introduced the simpleTaskTests and uploadTaskTests files which have the linker error.

The link failure may be related to llvm/llvm-project#49025. There's a workaround (see llvm/llvm-project#49025 (comment)) which probably works in this case, too?

It'd be a matter of adding this:

#if defined(__OBJC__) && defined(__clang__) && defined(_MSC_VER)
id __work_around_clang_bug = @"__unused__";
#endif

to simpleTaskTests.m and uploadTaskTests.m.

As a sidenote, I find that CI is most likely to be consistently green when all changes must go through pull requests, and CI is required to pass before a pull request can be merged. There's a fair amount of commits which have been pushed to the master branch without going through the PR process, and ended up breaking CI in one shape or form.

@hmelder
Copy link
Contributor Author

hmelder commented Sep 9, 2024

which probably works in this case, too?

Never worked for me, but I'll try :)

@hmelder
Copy link
Contributor Author

hmelder commented Sep 9, 2024

I don't understand why the linker discards the Objective-C sections. There should be enough Objective-C code in the unit tests... It builds with a newer version of lld-link too.

Edit: But this is only the case when GS_HAVE_NSURLSESSION is 1.

@hmelder
Copy link
Contributor Author

hmelder commented Sep 9, 2024

As a sidenote, I find that CI is most likely to be consistently green when all changes must go through pull requests, and CI is required to pass before a pull request can be merged. There's a fair amount of commits which have been pushed to the master branch without going through the PR process, and ended up breaking CI in one shape or form.

CI in #422 passed for all targets except MiNGW GCC which was broken at that time:
https://github.com/gnustep/libs-base/actions/runs/10177647029/job/28149581762

@qmfrederik
Copy link
Contributor

CI in #422 passed for all targets except MiNGW GCC which was broken at that time

That's odd. Perhaps something subtle changed in a new version of LLVM, which exposes this? It's a bit difficult to analyze CI failures in this project because CI often fails. I think it's easier to do root cause analysis of build failures in projects which consistently use CI and require builds to be CI.

which probably works in this case, too?
Never worked for me, but I'll try :)

Looks like it did work this time? 😄

Really happy to see CI gradually becoming greener, thanks!

@hmelder
Copy link
Contributor Author

hmelder commented Sep 9, 2024

Seems like we still have a deadlock in the NSThread test on MinGW GCC

@triplef
Copy link
Member

triplef commented Sep 9, 2024

Edit: But this is only the case when GS_HAVE_NSURLSESSION is 1.

Good catch. But should this not be the case for the MSVC CI?

@hmelder
Copy link
Contributor Author

hmelder commented Sep 9, 2024

But should this not be the case for the MSVC CI?

Yes but the CI decided it no longer wants to use libcurl ^^

checking for libcurl... FAILED (libcurl not found via pkg-config)

@qmfrederik
Copy link
Contributor

checking for libcurl... FAILED (libcurl not found via pkg-config)

I believe that libcurl is currently a 'soft' dependency, i.e. libs-base will just build without curl support if it didn't find it. Perhaps we should upgrade that to a 'hard' dependency, and forcing users to pass an explicit --without-curl flag if they don't want the curl support? That would cause CI to fail whenever it "forgets" to pick up curl.

(Sorry for the CI rants ^^)

@hmelder
Copy link
Contributor Author

hmelder commented Sep 9, 2024

Perhaps we should upgrade that to a 'hard' dependency, and forcing users to pass an explicit --without-curl flag if they don't want the curl support?

The only API that uses libcurl internally is NSURLSession which is only available when using the clang toolchain. Making it a hard dependency on GCC does not make sense. So perhaps a hard-dependency when using the libobjc2/clang toolchain?

@hmelder hmelder changed the title Fix CI Fix CI and make libcurl a hard-dependency when using libobjc2 Sep 9, 2024
@hmelder
Copy link
Contributor Author

hmelder commented Sep 9, 2024

Took some time to install the MinGW environment on poor ICE Wifi, but the culpit for the deadlock is the late_unregister.m NSThread test.

@hmelder
Copy link
Contributor Author

hmelder commented Sep 9, 2024

Everything except MinGW GCC works now. I cannot reproduce the deadlock and cannot get any logs out of the CI, as we use a test suite (a.k.a spaghetti shell code) that is unable to restrict the runtime of individual unit tests -_-

@hmelder
Copy link
Contributor Author

hmelder commented Sep 10, 2024

@qmfrederik can you take a look at late_unregister.m? It is randomly failing on MinGW.

@hmelder hmelder marked this pull request as draft September 17, 2024 16:12
@hmelder hmelder marked this pull request as ready for review September 24, 2024 16:54
@hmelder
Copy link
Contributor Author

hmelder commented Sep 24, 2024

I'd suggest turn the CI green first, and address the spurious failures in NSThread late_unregister.m and NSProxy separately, tracking it in an issue.

Otherwise, we continue merging PRs blindly without proper testing.

@hmelder hmelder merged commit 273776a into master Oct 10, 2024
8 checks passed
@hmelder hmelder deleted the fix-ci branch October 10, 2024 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants