-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat(cli): expose TAURI_TARGET_TRIPLE
to before*Commands, closes #5091
#5101
Conversation
TAURI_TARGET_TRIPLET
to before*Commands, closes #5091TAURI_TARGET_TRIPLE
to before*Commands, closes #5091
_ => {} | ||
} | ||
"linux" => env.insert("TAURI_PLATFORM_TYPE", "Linux".into()), | ||
"windows" => env.insert("TAURI_PLATFORM_TYPE", "Windows_NT".into()), |
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.
should this return Windows
instead?
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.
the previous code returned Windows_NT so we can't change 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.
add a todo for v2 then?
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.
sure
env.insert( | ||
"TAURI_ARCH", | ||
match arch { | ||
// keeps compatibility with old `std::env::consts::ARCH` implementation | ||
"i686" | "i586" => "x86".into(), | ||
a => a.into(), | ||
}, | ||
); | ||
env.insert( | ||
"TAURI_PLATFORM", | ||
match host { | ||
// keeps compatibility with old `std::env::consts::OS` implementation | ||
"darwin" => "macos".into(), | ||
"ios-sim" => "ios".into(), | ||
"androideabi" => "android".into(), | ||
h => h.into(), | ||
}, | ||
); |
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.
but the old implementation was just wrong and I expect TAURI_ARCH
will be used in build tools so for example darwin
, androideabi
will be the correct value.
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.
they can use the target triple for that. we can't break this, someone is probably using this (specially the platform one).
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 would make sense for the platform but for the arch, I don't like it tbh and I think users should be aware that they are using the wrong arch.
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.
tbh I see this as a bugfix and not breaking but lets add TODO for v2 then
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.
what's the difference between i686 and x86? it's the same information, and x86 is the format we were using already. I think you're mixing the arch and the platform.
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.
one could be trying to download a sidecar (built with rust) at build time using the arch, and maybe they ended up converting x86
to i686
or i586
(if someone still using that) but x86
wasn't our intended result in the first place so this should be considered a fix because we are delivering the intended behavior even if it breaks their build script.
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.
they can easily map that back into i686 or use the target triple directly (which is the usual way of handling this).
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.
they probably are but they can't map it back to i586 for example and they would need to use the target triple but that's beside the point. I simply don't want to wait until v2 to change this wrong behavior and maybe more people will end up depending on this wrong behavior. Also we are changing the tauri-cli which is just a tool and we are not breaking the framework.
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Checklist
fix: remove a typo, closes #___, #___
)Other information