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

Change naming convention of object files to follow what Base Julia does; also, start testing on macOS again (but only Intel macOS) #930

Merged
merged 3 commits into from
Oct 16, 2024

Conversation

gbaraldi
Copy link
Member

Fixes #738

I'm not sure why this makes macos broken, but given that the fix is simple I'm also not going to dig into it.

The tests don't all pass because fooifier isn't there for macos, @staticfloat would need to rebuild it or we just switch to another random artifact.

It also prints a bunch of

ld: warning: no platform load command found in '/var/folders/fs/z9fkqy2n5djg4663njw_c20m0000gn/T/jl_4TmZI5eNyL-o.a[2](text#0.o)', assuming: macOS
ld: warning: no platform load command found in '/var/folders/fs/z9fkqy2n5djg4663njw_c20m0000gn/T/jl_4TmZI5eNyL-o.a[3](text#1.o)', assuming: macOS
ld: warning: no platform load command found in '/var/folders/fs/z9fkqy2n5djg4663njw_c20m0000gn/T/jl_4TmZI5eNyL-o.a[4](text#2.o)', assuming: macOS
ld: warning: no platform load command found in '/var/folders/fs/z9fkqy2n5djg4663njw_c20m0000gn/T/jl_4TmZI5eNyL-o.a[5](text#3.o)', assuming: macOS
ld: warning: no platform load command found in '/var/folders/fs/z9fkqy2n5djg4663njw_c20m0000gn/T/jl_4TmZI5eNyL-o.a[6](text#4.o)', assuming: macOS

Which was fixed by JuliaLang/julia#51830 but that didn't get backported into 1.10.

@staticfloat
Copy link
Member

For the fooifier stuff, probably best to just use a JLL like HelloWorldC_jll or something like that. We don't want to actually add it as a dependency though, we want to embed the Artifacts.toml file directly, to continue testing the artifacts handling, so do something like just download an Artifacts.toml file from a JLL and embed it directly into the test suite here.

Copy link
Collaborator

@sloede sloede left a comment

Choose a reason for hiding this comment

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

I am really happy to see this issue fixed, thanks a lot, though I truly am baffled by how this fix is supposed to work 😬

Just two small remarks:

  1. The ld warning seems to appear only on macOS platforms, right? Can we maybe add an @info statement on macOS systems immediately before or after, informing the user that if said warning appears, it is ok to ignore?
  2. The rationale for using the -o.a extension is very subtle. It would be imho great to add a small comment that explains why this is necessary (and/or links to this PR).

@DilumAluthge
Copy link
Member

@gbaraldi What's the status here? We just need to fix the fooifier problem?

@DilumAluthge
Copy link
Member

Bump @gbaraldi @staticfloat

What needs to be done to fix CI here?

src/PackageCompiler.jl Show resolved Hide resolved
src/PackageCompiler.jl Show resolved Hide resolved
@DilumAluthge
Copy link
Member

The rationale for using the -o.a extension is very subtle. It would be imho great to add a small comment that explains why this is necessary (and/or links to this PR).

This is done now: 9387406

gbaraldi and others added 3 commits October 15, 2024 19:32
Thus, future readers of the source code will understand why we use this naming convention.

[Applies suggestions from code review.]
@DilumAluthge
Copy link
Member

Note: I have merged #967 into this PR.

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.56%. Comparing base (4d9da91) to head (71e9274).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #930      +/-   ##
==========================================
+ Coverage   83.71%   84.56%   +0.85%     
==========================================
  Files           3        3              
  Lines         823      823              
==========================================
+ Hits          689      696       +7     
+ Misses        134      127       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DilumAluthge
Copy link
Member

CI is all green.

@DilumAluthge DilumAluthge merged commit 467b90b into master Oct 16, 2024
21 checks passed
@DilumAluthge DilumAluthge deleted the gb/macos-is-odd branch October 16, 2024 00:47
@DilumAluthge DilumAluthge changed the title Change naming convention of object files to follow what Base Julia does Change naming convention of object files to follow what Base Julia does; also, start testing on macOS again Oct 16, 2024
@DilumAluthge DilumAluthge changed the title Change naming convention of object files to follow what Base Julia does; also, start testing on macOS again Change naming convention of object files to follow what Base Julia does; also, start testing on (Intel) macOS again Oct 16, 2024
@DilumAluthge DilumAluthge changed the title Change naming convention of object files to follow what Base Julia does; also, start testing on (Intel) macOS again Change naming convention of object files to follow what Base Julia does; also, start testing on macOS again (but only Intel macOS) Oct 16, 2024
@DilumAluthge DilumAluthge linked an issue Oct 16, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix CI failures on macOS macOS: System image file fails consistency check on binary startup
4 participants