-
Notifications
You must be signed in to change notification settings - Fork 68
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 shards regression #487
Fix shards regression #487
Conversation
fe3d85a
to
46f9c46
Compare
"x86_64-w64-mingw32"=>"x86_64-w64-windows-gnu", | ||
) | ||
|
||
triple2target(triple::String) = get(JLL_ENV_CLANG_TARGETS_MAPPING, triple, "unknown") | ||
|
||
function __triplet(p::Platform) | ||
for k in keys(p.tags) | ||
if k != "arch" && k != "os" && k != "libc" | ||
if k != "arch" && k != "os" && k != "libc" && k != "call_abi" && k != "os_version" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call_abi
is needed for armv7l, os_version
is needed for freebsd and sometimes darwin
t *= "14" | ||
elseif os(p) == "macos" && arch(p) == "aarch64" | ||
t *= "20" | ||
if os_version(p) === nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why the user wouldn't specify the version, but here I am just maintaining backward compatibility if they didn't.
looks like there are download issues on windows platforms. |
@testset "get_default_args for all triples" begin | ||
# just testing this does not crash on any default triple | ||
Clang.get_default_args.(Clang.JLLEnvs.JLL_ENV_TRIPLES) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test downloads artifacts for all platforms. running it every time is a bit overkill.
is there a way to somehow cache it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe we can control it by env variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry i didn't realize it did that, i've simplified the test
I'm not sure what that error means, do you know? |
no. probably just a mysterious connection issue. |
The new test looks good. Thanks! |
just noticed we just hit 1000 commits ;) |
This PR is my attempt at fixing the shards regression in #486. I added a test that checks all the default triples.
Fixes #486.