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

feat(cli): expose TAURI_TARGET_TRIPLE to before*Commands, closes #5091 #5101

Merged
merged 12 commits into from Oct 3, 2022

Conversation

amrbashir
Copy link
Member

@amrbashir amrbashir commented Aug 30, 2022

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@amrbashir amrbashir requested a review from a team as a code owner August 30, 2022 18:12
@lucasfernog lucasfernog changed the title feat(cli): expose TAURI_TARGET_TRIPLET to before*Commands, closes #5091 feat(cli): expose TAURI_TARGET_TRIPLE to before*Commands, closes #5091 Oct 3, 2022
_ => {}
}
"linux" => env.insert("TAURI_PLATFORM_TYPE", "Linux".into()),
"windows" => env.insert("TAURI_PLATFORM_TYPE", "Windows_NT".into()),
Copy link
Member Author

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?

Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

sure

Comment on lines +220 to +237
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(),
},
);
Copy link
Member Author

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.

Copy link
Member

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).

Copy link
Member Author

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.

Copy link
Member Author

@amrbashir amrbashir Oct 3, 2022

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

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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).

Copy link
Member Author

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.

@lucasfernog lucasfernog merged commit a4aec9f into dev Oct 3, 2022
@lucasfernog lucasfernog deleted the fix-cli-env-vars branch October 3, 2022 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants