Skip to content

druntime-test: Detect musl more robustly#21741

Merged
dlang-bot merged 1 commit intodlang:masterfrom
the-horo:fix-musl-detection
Aug 26, 2025
Merged

druntime-test: Detect musl more robustly#21741
dlang-bot merged 1 commit intodlang:masterfrom
the-horo:fix-musl-detection

Conversation

@the-horo
Copy link
Copy Markdown
Contributor

Try to detect musl on more systems than just Alpine.

This also fixes an issue in the Github runners where the default shell is dash which doesn't understand the &> operator leading to the musl-excluded tests to also be skipped on glibc linux.

@dlang-bot
Copy link
Copy Markdown
Contributor

Thanks for your pull request and interest in making D better, @the-horo! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#21741"

@the-horo
Copy link
Copy Markdown
Contributor Author

Spotted this in the ldc CI: ldc-developers/ldc#4969 (comment)

@the-horo
Copy link
Copy Markdown
Contributor Author

One issue with this PR is that it now treats IS_MUSL=0 as musl being present, which I need to fix.


ifeq (linux,$(OS))
LDD = ldd
IS_MUSL != $(LDD) --version 2>&1 | grep -q musl && echo 1
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's IMO some weird/unintuitive syntax; found it in https://www.gnu.org/software/make/manual/html_node/Setting.html. I think the $(shell …) wrapper is easier to understand.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reason for using != is that it's part of posix, not a gnu extension. It's also documented on https://www.gnu.org/software/make/manual/html_node/Shell-Function.html.

I don't object to changing it, if the gnu syntax is preferred, since the makefiles already rely on it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, so that != is actually standard, whereas $(shell) is an extension?! - I count 3 != occurrences in DMD master Makefiles (all Valgrind-related), and 16 $(shell).

No too strong opinion from my side, except that I find the != extremely unintuitive.

@kinke
Copy link
Copy Markdown
Contributor

kinke commented Aug 25, 2025

One issue with this PR is that it now treats IS_MUSL=0 as musl being present, which I need to fix.

No strong opinion from my side, we can also go with defined/undefined instead of checking for a 1 - the syntax with ifdef/ifndef seems a bit nicer to read to me (i.e., what you have right now).

@kinke
Copy link
Copy Markdown
Contributor

kinke commented Aug 25, 2025

Oh and please document the new variable in common.mak.

@the-horo
Copy link
Copy Markdown
Contributor Author

One issue with this PR is that it now treats IS_MUSL=0 as musl being present, which I need to fix.

No strong opinion from my side, we can also go with defined/undefined instead of checking for a 1 - the syntax with ifdef/ifndef seems a bit nicer to read to me (i.e., what you have right now).

My only concern is changing something that worked before. However, dmd and ldc should be the only ones that touch these files so, if you're fine with it, I'll keep the syntax.

@the-horo the-horo force-pushed the fix-musl-detection branch from 01e0354 to 82d4946 Compare August 25, 2025 19:24
Try to detect musl on more systems than just Alpine.

This also fixes an issue in the Github runners where the default shell
is dash which doesn't understand the `&>` operator leading to the
musl-excluded tests to also be skipped on glibc linux.

Signed-off-by: Andrei Horodniceanu <a.horodniceanu@proton.me>
@the-horo the-horo force-pushed the fix-musl-detection branch from 82d4946 to b3fca91 Compare August 25, 2025 20:07
@dlang-bot dlang-bot merged commit 61dbcba into dlang:master Aug 26, 2025
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants