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

Windows: Install MIGW dependency for x86_64-pc-windows-gnu #10

Closed
georgik opened this issue Oct 7, 2022 · 6 comments · Fixed by #27 or #43
Closed

Windows: Install MIGW dependency for x86_64-pc-windows-gnu #10

georgik opened this issue Oct 7, 2022 · 6 comments · Fixed by #27 or #43
Assignees
Labels
enhancement New feature or request

Comments

@georgik
Copy link
Contributor

georgik commented Oct 7, 2022

Install MIGW dependency for x86_64-pc-windows-gnu.

Without this dependency, the user will get an error during builds like

How to simulate

Run the following command in Windows Sandbox:

 .\espup.exe install

Solution

Implemented here: https://github.com/espressif/idf-env/blob/main/src/rust.rs#L484

Consider pointing directly to mingw repo, instead of esp-rs zipped version. The crate sevenz-rust for unpacking 7zip was improved and contains support also for unpacking Windows binaries: dyz1990/sevenz-rust#1

The new version of sevenz-rust should make it possible to remove this workaround: https://github.com/espressif/idf-env/blob/main/src/rust.rs#L182

This command should be deployed before any cargo install takes place.

@georgik georgik added the enhancement New feature or request label Oct 7, 2022
@SergioGasquez SergioGasquez self-assigned this Oct 7, 2022
@SergioGasquez
Copy link
Member

As a summary for those who are not familiar with this issue (or #9) or with Rust in Windows, there are two ABIs available for Windows:

#9 and #10 are the result of not having any of the proper ABI installed, and therefore, not meeting the Compile-time Requirements.

At the moment, in espup, we are checking if rustup is installed, and if is not, we install it, but we do check if the ABI is installed. With #9 and #10 we would add an argument to espup so that users can install the desired ABI.

So, I see two ways of dealing with this:

  • Add rustup as a requirement for espup:
    • We would document it as a requirement and add the proper resources so users can install it, although, I would assume that most of the users that get to espup already have rustup installed.
    • In the code, we should still check if rustup is installed, but this time, if we do not find it we should just bail and tell the user to install it.
    • We could also open an issue in rustup to try to get GNU ABI to behave like MSVC, where rustup could detect if installed and allow installing it if is not installed.
  • Allowing the user to install the ABIs from espup:

I would go for the option where we add rustup as a requirement since it avoids duplicating some logic from rustup and avoids the burden of having to keep the ABIs requirements up to date. This option would be even better if rustup added some checks for GNU requirements and the option to install them.

Please, let me know your opinion or if I am missing something.

Any other opinion/option is also more than welcome! @georgik @jessebraham @bjoernQ @MabezDev @JurajSadel

@bjoernQ
Copy link

bjoernQ commented Oct 11, 2022

I'd say option one (rustup as a requirement) is a very good option. As you said most users will already have it. Also, I'd say that if someone is willing to use GNU ABI those users are already used to the pain 😄

@jessebraham
Copy link
Member

@SergioGasquez and I discussed this yesterday and I also am of the opinion that the first option is the better one.

@SergioGasquez SergioGasquez linked a pull request Oct 18, 2022 that will close this issue
@georgik
Copy link
Contributor Author

georgik commented Nov 4, 2022

rustup deployment is integral part of vanilla boostrap environment. Please add the bootstrapping mechanism back.

@SergioGasquez SergioGasquez reopened this Nov 4, 2022
@georgik
Copy link
Contributor Author

georgik commented Nov 4, 2022

Thank you @SergioGasquez for returning the feature back.
Windows will be resolved the next week, we might be able to leverage winget tool which can treat VC Tools, MSYS2 and other stuff like pre-requisities and can reduce amount of code that we need to write.

@SergioGasquez SergioGasquez linked a pull request Nov 4, 2022 that will close this issue
@jessebraham jessebraham moved this to Todo in esp-rs Nov 4, 2022
@SergioGasquez
Copy link
Member

As discussed in #154, espup won't install this dependency.

@SergioGasquez SergioGasquez closed this as not planned Won't fix, can't repro, duplicate, stale Feb 15, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in esp-rs Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants