Skip to content

Bug fix for dragonfly and CI tests. #776

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
Merged
merged 1 commit into from
Jun 10, 2022
Merged

Bug fix for dragonfly and CI tests. #776

merged 1 commit into from
Jun 10, 2022

Conversation

Alexhuszagh
Copy link
Contributor

Added build-std flag to CI, so for targets that support std but are tier 3 targets, we can use the experimental build-std support to ensure they build. Also patches linking on dragonfly to ensure we use libgcc_s instead of libgcc_pic, which no longer exists. We just create a symlink for that purpose.

Necessary to merge #775.

@Alexhuszagh Alexhuszagh added bug A-bsd Area: BSD-family targets labels Jun 9, 2022
@Alexhuszagh Alexhuszagh requested a review from a team as a code owner June 9, 2022 21:32
@Alexhuszagh
Copy link
Contributor Author

bors try --target *-dragonfly

bors bot added a commit that referenced this pull request Jun 9, 2022
@bors
Try #776: --target *-dragonfly
4227214
@Alexhuszagh Alexhuszagh added the no changelog A valid PR without changelog (no-changelog) label Jun 9, 2022
@bors
Copy link
Contributor

bors bot commented Jun 9, 2022

try

Build succeeded:

@Emilgardis
Copy link
Member

it's not working

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jun 9, 2022 •
edited
Loading

Checking it, yeah don't merge it yet. OK the first part works, the part of the test I changed. But other, further parts of test.sh don't work. I have the local image so I'll fix this.

@Emilgardis Emilgardis self-requested a review June 9, 2022 22:01
@Alexhuszagh
Copy link
Contributor Author

Ok I think I've fixed everything. I've just changed the logic so instead of cross, we use ${CROSS[@]} and "${CROSS_FLAGS[@]}" everywhere, which then sets the toolchain version and if we need -Zbuild-std. It works locally on my machine.

bors try --target x86_64-unknown-dragonfly --target arm-unknown-linux-gnueabihf --target thumbv6m-none-eabi

bors bot added a commit that referenced this pull request Jun 9, 2022
@bors
Try #776: --target x86_64-unknown-dragonfly --target arm-unknown-linu…
c7233cd 
…x-gnueabihf --target thumbv6m-none-eabi
@Emilgardis
Copy link
Member

im not a fan of this, instead I think it'd suffice if we make

cross/src/lib.rs

Lines 352 to 354 in ee2fc1b

let uses_xargo = config
.xargo(&target)?
.unwrap_or_else(|| !target.is_builtin() || !available_targets.contains(&target));
actually properly determine if build-std is needed, and then provide the needed flags if possible. Obviously, that's a bigger change than this simple fix, but I think that's the long-term solution, especially if we add more t3 targets to be "builtins"

@Alexhuszagh
Copy link
Contributor Author

im not a fan of this, instead I think it'd suffice if we make

cross/src/lib.rs

Lines 352 to 354 in ee2fc1b

let uses_xargo = config
.xargo(&target)?
.unwrap_or_else(|| !target.is_builtin() || !available_targets.contains(&target));

actually properly determine if build-std is needed, and then provide the needed flags if possible. Obviously, that's a bigger change than this simple fix, but I think that's the long-term solution, especially if we add more t3 targets to be "builtins"

Probably a more logical solution, should I incorporate this into this PR then? It's probably better to have this logic outside our test suite for sure.

@Emilgardis
Copy link
Member

I'm fine with merging this already or with the suggested "real" fix.

@bors
Copy link
Contributor

bors bot commented Jun 9, 2022

try

Build succeeded:

Emilgardis
Emilgardis approved these changes Jun 9, 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+

bors bot added a commit that referenced this pull request Jun 9, 2022
@bors @Jules-Bertholet @Alexhuszagh
Merge #775 #776
060a0f2 
775: Forward Cargo exit code r=Emilgardis a=Jules-Bertholet

Forward Cargo's exit code. For example, if `cargo test` returns a non-zero exit code due to a failing test, `cross` will also return a non-zero exit code.

776: Bug fix for dragonfly and CI tests. r=Emilgardis a=Alexhuszagh

Added `build-std` flag to CI, so for targets that support `std` but are tier 3 targets, we can use the experimental `build-std` support to ensure they build. Also patches linking on dragonfly to ensure we use `libgcc_s` instead of `libgcc_pic`, which no longer exists. We just create a symlink for that purpose.

Necessary to merge #775.

Co-authored-by: Jules Bertholet <julesbertholet@quoi.xyz>
Co-authored-by: Alex Huszagh <ahuszagh@gmail.com>
@bors
Copy link
Contributor

bors bot commented Jun 9, 2022

This PR was included in a batch that was canceled, it will be automatically retried

@Emilgardis
Copy link
Member

bors r-

@bors
Copy link
Contributor

bors bot commented Jun 9, 2022

Canceled.

@Alexhuszagh Alexhuszagh force-pushed the dragonfly_patch branch 2 times, most recently from 4559c61 to f5455ea Compare June 10, 2022 00:25
Emilgardis
Emilgardis reviewed Jun 10, 2022
View reviewed changes
@Alexhuszagh
Bug fix for dragonfly and CI tests.
327345f 
Added `build-std` flag to CI, so for targets that support `std` but are
tier 3 targets, we can use the experimental `build-std` support to
ensure they build. Also patches linking on dragonfly to ensure we use
`libgcc_s` instead of `libgcc_pic`, which no longer exists. We just
create a symlink for that purpose.

This also adds `build-std` detection into Rust, so we don't have to
provide it manually. It allows use to avoid using `xargo` when possible.

Necessary to merge cross-rs#775.
@Alexhuszagh
Copy link
Contributor Author

So it turns out build-std detection I think is impossible (?), so this really should be a config option. Xargo gets around it by requiring you to enable std during the build, and we can't guarantee tier 3 targets will have a std library. So, I think build-std needs to be set as a config variable (#774 simplifies this) which takes priority over xargo, with the whole environment variable/target/build environment setup.

bors try --target x86_64-unknown-dragonfly --target arm-unknown-linux-gnueabihf --target thumbv6m-none-eabi --target x86_64-apple-darwin

bors bot added a commit that referenced this pull request Jun 10, 2022
@bors
Try #776: --target x86_64-unknown-dragonfly --target arm-unknown-linu…
65a0c13 
…x-gnueabihf --target thumbv6m-none-eabi --target x86_64-apple-darwin
@bors
Copy link
Contributor

bors bot commented Jun 10, 2022

try

Build succeeded:

@Emilgardis
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 10, 2022

Build succeeded:

@bors bors bot merged commit d8034d4 into cross-rs:main Jun 10, 2022
@Emilgardis Emilgardis added this to the v0.2.2 milestone Jun 15, 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 approved these changes

Assignees
No one assigned
Labels
A-bsd Area: BSD-family targets bug no changelog A valid PR without changelog (no-changelog)
Projects
None yet
Milestone
v0.2.2
Development

Successfully merging this pull request may close these issues.
3 participants
@Alexhuszagh @Emilgardis @otavio