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.

Sign up for GitHub

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

Jump to bottom

Allow setting default build target #624

Merged
merged 4 commits into from
Mar 24, 2022
Merged

Allow setting default build target #624

merged 4 commits into from
Mar 24, 2022

Conversation

zappolowski
Copy link
Contributor

This implements #368

This feature allows to specify the default target to build either in Cross.toml

[build]
target = "aarch64-unknown-linux-gnu"

or by setting CROSS_BUILD_TARGET to avoid the need to always specify --target.

I've opted for build.target as this resembles the configuration of cargo.

@zappolowski zappolowski requested review from Dylan-DPC-zz and a team as code owners January 8, 2022 13:23
@zappolowski zappolowski marked this pull request as draft January 8, 2022 15:57
@zappolowski zappolowski force-pushed the feature/default-targets branch from ce91128 to 22ab4d1 Compare January 8, 2022 16:30
@zappolowski zappolowski marked this pull request as ready for review January 12, 2022 16:49
@zappolowski zappolowski force-pushed the feature/default-targets branch from 22ab4d1 to c226dcb Compare February 13, 2022 07:59
@zappolowski zappolowski requested a review from a team as a code owner February 13, 2022 07:59
@zappolowski zappolowski force-pushed the feature/default-targets branch from c226dcb to 011a6b4 Compare February 13, 2022 07:59
Emilgardis
Emilgardis previously approved these changes Feb 13, 2022
View reviewed changes
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

Looks good! Cant merge right now due to CI being incomplete

@zappolowski
Copy link
Contributor Author

Cant merge right now due to CI being incomplete

Is some action required on my site to complete the CI?

@Emilgardis
Copy link
Member

No, see #609 <3

@reitermarkus
Copy link
Member

Does this need to be in Cross.toml? Can't this be in .cargo/config.toml and we automatically detect it?

@zappolowski
Copy link
Contributor Author

zappolowski commented Mar 11, 2022 •
edited
Loading

Does this need to be in Cross.toml? Can't this be in .cargo/config.toml and we automatically detect it?

I've opted for using Cross.toml in the first place as it's a configuration made for cross and shouldn't affect cargo. Another issue was/is that according to the cargo documentation the actual value can be defined in several places and duplicating the resolution implemented by cargo (and keeping it up to date) seemed unnecessary complex.

My proposal would be to postpone this feature until cargo config is stabilized as this should make it possible to just parse the output of cargo config get --format toml build.target and use this without the hassle mentioned above.

@reitermarkus
Copy link
Member

My proposal would be to postpone this feature until rust-lang/cargo#9301 is stabilized

Sounds fair.

Ideally I would like to get rid of Cross.toml eventually and use package.metadata.cross in Cargo.toml to store cross configuration.

otavio
otavio previously approved these changes Mar 17, 2022
View reviewed changes
@otavio
Copy link
Contributor

otavio commented Mar 17, 2022

I don't see why this should be postponed as this is very useful and a valid use case. Once the cargo feature is merged this can be reworked in a new release.

@Emilgardis Emilgardis mentioned this pull request Mar 17, 2022
Closed
@mntns mntns mentioned this pull request Mar 22, 2022
Merged
@zappolowski zappolowski dismissed stale reviews from otavio and Emilgardis via 48028da March 22, 2022 19:22
@zappolowski zappolowski force-pushed the feature/default-targets branch from 011a6b4 to 48028da Compare March 22, 2022 19:22
@zappolowski zappolowski removed the request for review from Dylan-DPC-zz March 22, 2022 19:23
bors bot pushed a commit that referenced this pull request Mar 22, 2022
@mntns
Add field for default target to build, in relation to #624
2937acc
bors bot added a commit that referenced this pull request Mar 22, 2022
@bors @mntns
Merge #670
b40f3fc 
670: Implement `Cross.toml` deserialization using `serde` r=Emilgardis a=mntns

This PR implements the deserialization of the `Cross.toml` config using `serde`. It can therefore be seen as some preliminary work on the issues #657, #664, and is related to #532 on #624.

I added some basic documentation as module-level documentation, although I'm not sure whether this is the best place for that. Ideally it should be documentated in the `README.md` at some point. Furthermore, `CrossToml` lives in its own module, but a more suitable place might be the `config` module.

Co-authored-by: Niklas Kunz <[email protected]>
@Emilgardis
Copy link
Member

can you please rebase this again so we can get this merged? There's been some changes with #670

@zappolowski
Initial infrastructure for setting default target
2f28114 
The goal is to allow setting the default target to build. This can be
done either in `Cross.toml` via

```toml
[build]
target = "armv7-unknown-linux-musleabihf"
```

or by setting `CROSS_BUILD_TARGET` to the desired value.
@zappolowski
Allow setting default target via environment
a69752a
@zappolowski
Allow setting default target via Cross.toml
cbb2382 
N.B. `Debug` is needed for `TargetList` as linting rules demand it.
@zappolowski zappolowski force-pushed the feature/default-targets branch from 48028da to cbb2382 Compare March 23, 2022 18:17
@Emilgardis Emilgardis linked an issue Mar 24, 2022 that may be closed by this pull request
Ability to set default build target #368
Closed
@Emilgardis Emilgardis force-pushed the feature/default-targets branch from 912e074 to 4cb1f3e Compare March 24, 2022 18:24
@Emilgardis
Copy link
Member

I've renamed the option to default-target to better convey what it does

@Emilgardis
rename option to default-target
c84fce2 
also document the option
@Emilgardis Emilgardis force-pushed the feature/default-targets branch from 4cb1f3e to c84fce2 Compare March 24, 2022 18:27
Emilgardis
Emilgardis approved these changes Mar 24, 2022
View reviewed changes
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

bors r+

thanks!

@bors
Copy link
Contributor

bors bot commented Mar 24, 2022

Build succeeded:

@bors bors bot merged commit c4ee711 into cross-rs:main Mar 24, 2022
@zappolowski zappolowski deleted the feature/default-targets branch March 26, 2022 07:23
@Emilgardis Emilgardis added this to the v0.2.2 milestone Jun 15, 2022
@Alexhuszagh Alexhuszagh added enhancement no-ci-targets PRs that do not affect any cross-compilation targets. labels Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Emilgardis Emilgardis Emilgardis approved these changes

@otavio otavio otavio left review comments

Assignees
No one assigned
Labels
enhancement no-ci-targets PRs that do not affect any cross-compilation targets.
Projects
None yet
Milestone
v0.2.2
Development

Successfully merging this pull request may close these issues.

Ability to set default build target
5 participants
@zappolowski @Emilgardis @reitermarkus @otavio @Alexhuszagh