Skip to content

Test serialization fixes for macOS, Windows #14649

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lb90
Copy link
Contributor

@lb90 lb90 commented May 27, 2025

Add directories in PATH for depends like we do for args

Fixes #4668

@lb90 lb90 requested a review from jpakkane as a code owner May 27, 2025 09:16
@lb90 lb90 force-pushed the test-depends-get-windows-extra-path branch from 169a23d to 97c9cfa Compare May 27, 2025 09:26
@lb90 lb90 changed the title Draft: Test serialization: Set PATH also for depends Draft: Test serialization: Set PATH also for depends [Windows] May 27, 2025
@lb90 lb90 changed the title Draft: Test serialization: Set PATH also for depends [Windows] Test serialization: Set PATH also for depends [Windows] May 27, 2025
@lb90
Copy link
Contributor Author

lb90 commented May 27, 2025

Should I add a test?

@bruchar1 bruchar1 added OS:windows Winodows OS specific issues test targets labels May 27, 2025
@bruchar1
Copy link
Member

Should I add a test?

Yes, I think having a test would be great. I'm not sure how easy it can be however.

@lb90 lb90 force-pushed the test-depends-get-windows-extra-path branch 2 times, most recently from c37b852 to 137a8a9 Compare May 27, 2025 12:41
@bruchar1
Copy link
Member

If you add the test first, and then the fix, you could ensure that it isn't working before the fix...

@lb90 lb90 force-pushed the test-depends-get-windows-extra-path branch from 137a8a9 to 57f2d96 Compare May 27, 2025 12:50
@lb90
Copy link
Contributor Author

lb90 commented May 27, 2025

@bruchar1
Copy link
Member

Question; how should I address https://github.com/mesonbuild/meson/actions/runs/15275533157/job/42960871671?pr=14649#step:5:16?

You can probably do something like: extra_bdeps.extend(t.depends)

@lb90 lb90 force-pushed the test-depends-get-windows-extra-path branch from 57f2d96 to bcad352 Compare May 27, 2025 13:05
@lb90
Copy link
Contributor Author

lb90 commented May 27, 2025

Alright, test failure is here: https://github.com/mesonbuild/meson/actions/runs/15276070921/job/42962652326?pr=14649#step:5:320. I'm going to add the fix now :)

EDIT: better wait for the entire CI to finish...

@lb90
Copy link
Contributor Author

lb90 commented May 27, 2025

I can easily make the test cross-platform using dlopen + dlsym on UNIX, then it could be moved to common/. Would you be interested in that?

@bruchar1
Copy link
Member

I can easily make the test cross-platform using dlopen + dlsym on UNIX, then it could be moved to common/. Would you be interested in that?

If this is easy, yes, I have no objection. I guess it would test that the LD_LIBRARY_PATH is correctly populated, isn't it?

@lb90 lb90 force-pushed the test-depends-get-windows-extra-path branch 2 times, most recently from b5c9688 to ceb196d Compare May 28, 2025 09:21
@lb90
Copy link
Contributor Author

lb90 commented May 28, 2025

I guess it would test that the LD_LIBRARY_PATH is correctly populated, isn't it?

Yes 👍

Now I wonder if the test will pass on macOS...

EDIT: yeah, it's failing: https://github.com/mesonbuild/meson/actions/runs/15296442821/job/43026602442?pr=14649#step:7:583

@bruchar1
Copy link
Member

You can skip the test on Mac by adding

if host_machine.system() == 'darwin'
    error('MESON_SKIP_TEST reason for the skip')
endif

to the build file, and

"expect_skip_on_jobname": ["macos"]

in test.json

@lb90
Copy link
Contributor Author

lb90 commented May 28, 2025

Thanks :) The test is working an shows an actual issue on macOS, namely that Meson doesn't set LD_LIBRARY_PATH: https://github.com/mesonbuild/meson/blob/1.8.1/mesonbuild/backend/backends.py#L1293 (cfr #14430). So I'd repurpose this PR to fix both the macOS and Windows issues

@thesamesam thesamesam self-requested a review May 28, 2025 11:48
@bruchar1 bruchar1 removed the OS:windows Winodows OS specific issues label May 28, 2025
lb90 added 2 commits May 29, 2025 11:34
It's needed on Darwin for the same reason it's needed on
generic UNIX. Darwin supports both LD_LIBRARY_PATH and
DYLD_LIBRARY_PATH, but the two are not quite equivalent [1],
so we set both.

[1] https://github.com/ffi/ffi/blob/29ad900a/lib/ffi/dynamic_library.rb#L40
@lb90
Copy link
Contributor Author

lb90 commented May 29, 2025

Ok, all lights are green! I'm going to add the final commits now: first the test, then the necessary fixes

@lb90 lb90 changed the title Test serialization: Set PATH also for depends [Windows] Test serialization fixes for macOS, Windows May 29, 2025
@lb90 lb90 force-pushed the test-depends-get-windows-extra-path branch from b18c3bc to 6bfbb00 Compare May 29, 2025 10:30
Copy link
Member

@bruchar1 bruchar1 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you.

@lb90
Copy link
Contributor Author

lb90 commented May 29, 2025

Thanks a lot for your help!

@bruchar1 bruchar1 added this to the 1.8.2 milestone May 29, 2025
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.

test() dependencies not added to PATH
3 participants