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: support homebrew/linuxbrew (AppleClang, GCC 11) #3613

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Nov 18, 2024

This should detect the Linux failure discussed in #3495. Ref #3578.

This contains three four five fixes:

  • The deduced template guide is expanded explicitly for AppleClang. There's a note about AppleClang near it, with an expanded template, but this one wasn't expanded. It's required on macOS 13's AppleClang (14+ is fine)
  • I've added a missing FPIC when building shared libs. It doesn't link with the linuxbrew setup without this.
  • I've reordered the connection code so that the pre-declarations are on references rather than on using statements. GCC 11 seems to choke on the using statements being pre-declared, but references have been supported since medieval times or so. platform_list/channel_list is expanded explicitly.
  • Dependency on LibArchive's header was missing. If CMake 3.17 was the minimum instead of 3.16, this could use the LibArchive::LibArchive target instead
  • The new warning suppression from maint: Enable -Werror compiler flag for GCC, Clang and AppleClang #3611 broke GCC < 13 support. Restored (at least to GCC 11).

@henryiii henryiii force-pushed the henryiii/ci/brewtest branch 10 times, most recently from fc771cb to f7d45d9 Compare November 18, 2024 21:23
@henryiii
Copy link
Contributor Author

Open to opinions on how to do permissions better, that was a mess. :) This is now showing the current linux failure. I'll make a patch to fix next.

@henryiii henryiii force-pushed the henryiii/ci/brewtest branch 3 times, most recently from 7f68f72 to c6848ac Compare November 18, 2024 22:15
@henryiii henryiii marked this pull request as ready for review November 18, 2024 22:16
@henryiii
Copy link
Contributor Author

henryiii commented Nov 18, 2024

Commit moved to description.

@JohanMabille
Copy link
Member

JohanMabille commented Nov 20, 2024

Hi, sorry for the late reply. Would it be equivalent to add the AppleClang compiler to the already existing OSX job(s)? I think this would be simpler than setting up a new job from scratch.

@henryiii
Copy link
Contributor Author

henryiii commented Nov 20, 2024

This is for LinuxBrew, I didn't set up AppleClang at all in this PR (though I did include a fix for AppleClang, the other two fixes are for GCC 11.4 for LinuxBrew). I think this is the simplest way to add a quick LinuxBrew test.

I could take a stab at adding AppleClang.

@henryiii
Copy link
Contributor Author

(BTW, I'm calling brew on linux LinuxBrew, because that's what it used to be called, though I think these days it's just "brew on linux" as it now has first-class support.)

@henryiii henryiii force-pushed the henryiii/ci/brewtest branch 7 times, most recently from d878a4d to 971570a Compare November 20, 2024 21:37
@henryiii
Copy link
Contributor Author

FYI, it looks like C++17 is requested but 20 is required:

/Users/runner/work/mamba/mamba/libmamba/src/download/mirror_impl.cpp:183:38: warning: captured structured bindings are a C++20 extension [-Wc++20-extensions]

@henryiii henryiii force-pushed the henryiii/ci/brewtest branch 2 times, most recently from 179a32f to 141a102 Compare November 20, 2024 22:08
@henryiii
Copy link
Contributor Author

I think adding it as a new job is still much simpler, even for homebrew. I've added homebrew too, and fixed one more mistake.

@henryiii henryiii changed the title ci: add linuxbrew build fix: support homebrew/linuxbrew, add CI jobs for them Nov 20, 2024
@henryiii
Copy link
Contributor Author

This patch (adapted to 2.0.3) fixes brew: Homebrew/homebrew-core#195371

@jjerphan jjerphan added the release::bug_fixes For PRs fixing bugs label Nov 21, 2024
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Thank you for this PR, @henryiii.

Like @JohanMabille mentioned, I think we would welcome changes to the codebase to make micromamba distributable on more platform, but I do not think we can maintain another workflow (at least for now) given our priorities and constraints.

Hence I would discard the changes this PR makes to .github/workflows/, so rebasing onto or merging main should resolve it.

Also, #3611 should have fixed the problem raised in #3613 (comment).

@@ -20,6 +20,7 @@ find_package(Libsolv REQUIRED)

if(BUILD_SHARED)
set(LIBSOLV_DEPS solv::libsolv solv::libsolvext)
set_target_properties(solv-cpp PROPERTIES POSITION_INDEPENDENT_CODE ON)
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated: I think we better use PIC for all targets when BUILD_SHARED is set.

Copy link
Contributor Author

@henryiii henryiii Nov 21, 2024

Choose a reason for hiding this comment

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

This was the only one that complained, so the only one I fixed.

libmamba/CMakeLists.txt Outdated Show resolved Hide resolved
@henryiii
Copy link
Contributor Author

henryiii commented Nov 21, 2024

I do not think we can maintain another workflow

I was specifically asked to add this:

New GHA workflows can be set to test Homebrew's toolchains.

It caught multiple mistakes that the current tests didn't catch, and is shorter than the existing tests (4 and 5 minutes, while other jobs take 9+ minutes. #3578 takes 19 minutes), and even just found new mistakes introduced in #3611, so I think it is valuable - otherwise, I'm going to have to come back every release and make patches, which I don't wan to do. It's also much shorter than the existing workflows (5 mins). What about making it only run under certain circumstances, like if a tag is present or manually triggered?

This is the only test with AppleClang and the only test with GCC 11.4.

(I've reversed the order of the commits, and will pull this out into a separate PR so it can be a separate choice and not hold up bug fixes).

Signed-off-by: Henry Schreiner <[email protected]>
@henryiii
Copy link
Contributor Author

Tests passing (the new ones, that is), so will pull the CI change off to separate PR.

@henryiii henryiii changed the title fix: support homebrew/linuxbrew, add CI jobs for them fix: support homebrew/linuxbrew (AppleClang, GCC 11) Nov 21, 2024
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM given 🟢 CI checks.

I should have been clearer and have said that additions of or to workflows can be considered. Ideally we should extend the existing one, but let's discuss this in another Pull Request.

Thank you for your comprehension, @henryiii.

@henryiii
Copy link
Contributor Author

henryiii commented Nov 21, 2024

Adding it to the existing workflows added far more time due to parametrization and extra testing, and was much more complex than a simple extra workflow. It also couldn't catch certain problems, like the missing include. Even just swapping Clang for AppleClang was complicated due to the presets forcing every environment variable to be set

@JohanMabille
Copy link
Member

JohanMabille commented Nov 21, 2024

This is for LinuxBrew, I didn't set up AppleClang at all in this PR (though I did include a fix for AppleClang, the other two fixes are for GCC 11.4 for LinuxBrew). I think this is the simplest way to add a quick LinuxBrew test.

I could take a stab at adding AppleClang.

Ok, sorry for the confusion, I tought this was added to have a mean to test it on AppleClang (only), since HomeBrew uses it, and that testing AppleClang on OSX would have been equivalent (and simpler to set). Since this is testing more toolchains and catches more bugs, I agree it is valuable.

@JohanMabille JohanMabille merged commit 020b116 into mamba-org:main Nov 21, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release::bug_fixes For PRs fixing bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants