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

Support new NTO QNX targets #1312

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

flba-eb
Copy link
Contributor

@flba-eb flba-eb commented Dec 3, 2024

This change:

  • Adds support for two new targets
  • Adds support for a new target_env value
  • Gives users a bit more information where an error message comes from (e.g. if target is unknown)

This depends on rust-lang/rust/pull/133631

CC: @jonathanpallant @japaric @gh-tr @AkhilTThomas

@madsmtm
Copy link
Collaborator

madsmtm commented Dec 3, 2024

Thanks for the PR!

The generated.rs shouldn't be updated manually, it will be updated by a CI job the next Friday after the Rust PR merges.


I think this is the first time that a Rust target is added since #1225, I'd like to know what you think we can do better (apart from improving the error message) to make such work smoother in the future?

@flba-eb flba-eb force-pushed the add_nto-iosock_rust-master branch from 30ba3ec to 66cfc7d Compare December 4, 2024 07:09
@flba-eb
Copy link
Contributor Author

flba-eb commented Dec 4, 2024

Thanks for the PR!

The generated.rs shouldn't be updated manually, it will be updated by a CI job the next Friday after the Rust PR merges.

I think this is the first time that a Rust target is added since #1225, I'd like to know what you think we can do better (apart from improving the error message) to make such work smoother in the future?

Thanks for the feedback, I've reverted the changes of generated.rs.

Something that would have helped me (and I think @thejpster as well) is:

  • A note in https://rustc-dev-guide.rust-lang.org/building/new-target.html about this issue. It does mention some required changes in e.g. Cargo.toml to allow targets not yet supported by nightly Rust, but nothing about the cc crate.
  • A more clear error message of the cc crate. This PR tries to improve it a bit (so that the user at least knows where the error message is coming from), but e.g. a link to some additional documentation (like the dev guide of previous bullet point) could be even better.

@madsmtm
Copy link
Collaborator

madsmtm commented Dec 4, 2024

Thanks for the feedback! I've opened #1313 to improve the error message, would like your input there on how the dev guide could be improved too!

@flba-eb flba-eb force-pushed the add_nto-iosock_rust-master branch from 66cfc7d to eaff721 Compare December 5, 2024 07:02
@flba-eb
Copy link
Contributor Author

flba-eb commented Dec 5, 2024

The PR now only adds the new target_env definition. The error message is now improved with #1313.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Dec 5, 2024

The generated.rs shouldn't be updated manually, it will be updated by a CI job the next Friday after the Rust PR merges.

You can modify and run cargo run inside the dev-tools/gen-target-info https://github.com/rust-lang/cc-rs/tree/main/dev-tools/gen-target-info

It will generate this src/target/generated

@flba-eb flba-eb force-pushed the add_nto-iosock_rust-master branch from eaff721 to 3718de7 Compare December 18, 2024 08:51
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.

3 participants