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

panic when image does not exist, warnings/errors have been emitted and running on CI or with CROSS_NO_WARNINGS=1 #661

Merged
merged 4 commits into from
Jan 31, 2024
Merged

panic when image does not exist, warnings/errors have been emitted and running on CI or with CROSS_NO_WARNINGS=1 #661

merged 4 commits into from
Jan 31, 2024

Conversation

Emilgardis
Copy link
Member

No description provided.

@Emilgardis Emilgardis requested a review from a team as a code owner March 17, 2022 16:19
@Emilgardis Emilgardis linked an issue Mar 17, 2022 that may be closed by this pull request
error/panic on fallback to host cargo or when in CI environment #659
Closed
@Emilgardis Emilgardis force-pushed the ci_fail_host_callback branch 3 times, most recently from 1d3a8c8 to d837e8e Compare March 17, 2022 18:30
@Emilgardis
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Mar 17, 2022
@bors
Try #661:
fd607c4
@bors
Copy link
Contributor

bors bot commented Mar 17, 2022

try

Build failed:

@Emilgardis Emilgardis marked this pull request as draft March 19, 2022 10:23
Alexhuszagh
Alexhuszagh reviewed May 24, 2022
View reviewed changes
CHANGELOG.md Outdated
@@ -5,6 +5,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]

- #661 - Panic when image does not exist and running on CI or with `CROSS_FORCE_IMAGE=1`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is relatively pedantic, but bool::from_str doesn't work with 1, so it should be CROSS_FORCE_IMAGE=true, or else force_image is false.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, should only check for the existance of the variable, but not care about the value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should it accept a certain range of values (since setting it to false or 0 should be falsey, which true or 1 should be truthy) rather than merely check the existence of the variable? This looks like it could also be relevant to CROSS_DOCKER_IN_DOCKER.

Alexhuszagh added a commit to Alexhuszagh/cross that referenced this pull request May 26, 2022
@Alexhuszagh
Evaluate boolean environment variables as truthy or falsey.
c943b0b 
Allow values of 0, 1, so values other than `true` or `false` can be
provided. `VAR=0`, `VAR=`, `VAR=-0`, or `VAR=false` will evaluate to
false, while the rest will evaluate to true.

Affects cross-rs#661 and cross-rs#721.
@Alexhuszagh Alexhuszagh mentioned this pull request May 26, 2022
Merged
Alexhuszagh added a commit to Alexhuszagh/cross that referenced this pull request May 26, 2022
@Alexhuszagh
Evaluate boolean environment variables as truthy or falsey.
933c03f 
Allow values of 0, 1, so values other than `true` or `false` can be
provided. `VAR=0`, `VAR=`, `VAR=-0`, or `VAR=false` will evaluate to
false, while the rest will evaluate to true.

Affects cross-rs#661 and cross-rs#721.
bors bot added a commit that referenced this pull request May 26, 2022
@bors @Alexhuszagh
Merge #722
10da657 
722: Evaluate boolean environment variables as truthy or falsey. r=Emilgardis a=Alexhuszagh

Allow values of 0, 1, so values other than `true` or `false` can be provided. `VAR=0`, `VAR=`, `VAR=-0`, or `VAR=false` will evaluate to false, while the rest will evaluate to true.

Affects #661 and #721.

Co-authored-by: Alex Huszagh <[email protected]>
@Emilgardis Emilgardis mentioned this pull request May 27, 2022
Closed
Alexhuszagh
Alexhuszagh requested changes May 29, 2022
View reviewed changes
Copy link
Contributor

@Alexhuszagh Alexhuszagh left a comment

Choose a reason for hiding this comment

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

Very minor changes but this looks good.

src/cli.rs Outdated
@@ -65,6 +66,9 @@ pub fn parse(target_list: &TargetList) -> Args {
let docker_in_docker = env::var("CROSS_DOCKER_IN_DOCKER")
.map(|s| bool::from_str(&s).unwrap_or_default())
.unwrap_or_default();
let force_image = env::var("CROSS_FORCE_IMAGE")
.map(|s| bool::from_str(&s).unwrap_or_default())
Copy link
Contributor

Choose a reason for hiding this comment

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

This can now use bool_from_envvar, which should fix the issue mentioned in the README.

@Alexhuszagh
Copy link
Contributor

Does this still need to be a draft? Should be good to go, and since this is opt-in, we don't have to worry about breaking changes or anything.

@Emilgardis
Copy link
Member Author

it's not opt in on CI though, which is where this will be hit the most (for good reasons)

@Emilgardis
Copy link
Member Author

I think this should be included in 0.3.0, however as it is breaking in a subtle way, I think emitting a true "cargo" style warning (so that githubs and other CI's problem matchers pick it up) for deprecation/wrongness is good, we can make the env var force the behaviour, and later toggle the behaviour to always work

@Alexhuszagh
Copy link
Contributor

Now that we've merged breaking changes for v0.3.0, this is ready for merging once all the conflicts have been fixed.

@Emilgardis Emilgardis changed the title panic when image does not exist and running on CI or with CROSS_FORCE_IMAGE=1 panic when image does not exist and running on CI or with CROSS_NO_WARNINGS=1 Mar 24, 2023
@Emilgardis Emilgardis changed the title panic when image does not exist and running on CI or with CROSS_NO_WARNINGS=1 panic when image does not exist, warnings/errors have been emitted and running on CI or with CROSS_NO_WARNINGS=1 Mar 24, 2023
@Emilgardis Emilgardis added the no-ci-targets PRs that do not affect any cross-compilation targets. label Mar 24, 2023
@Emilgardis Emilgardis mentioned this pull request Mar 26, 2023
Closed
2 tasks
@Emilgardis Emilgardis added this to the v0.3.0 milestone Mar 27, 2023
@Emilgardis Emilgardis mentioned this pull request May 12, 2023
Open
11 tasks
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@Emilgardis
Copy link
Member Author

/ci try -t aarch64-unknown-linux-gnu

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@Emilgardis Emilgardis removed the no-ci-targets PRs that do not affect any cross-compilation targets. label Jan 31, 2024
@Emilgardis
Copy link
Member Author

/ci try

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@Emilgardis
Copy link
Member Author

/ci try

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions
Copy link

Try run for comment

Failed Jobs

Successful Jobs

List

@Emilgardis Emilgardis added no-ci-targets PRs that do not affect any cross-compilation targets. breaking change labels Jan 31, 2024
@Emilgardis Emilgardis added this pull request to the merge queue Jan 31, 2024
Merged via the queue into cross-rs:main with commit 0547637 Jan 31, 2024
23 checks passed
@Emilgardis Emilgardis deleted the ci_fail_host_callback branch January 31, 2024 22:05
@cameronvoell cameronvoell mentioned this pull request Feb 2, 2024
Merged
@Emilgardis Emilgardis mentioned this pull request Feb 6, 2024
Merged
har7an added a commit to har7an/cross that referenced this pull request Feb 13, 2024
@har7an
docs/env_var: Document missing variable from cross-rs#661.
fc4debb
har7an added a commit to har7an/cross that referenced this pull request Feb 13, 2024
@har7an
docs/env_var: Document missing variable from cross-rs#661.
f37f180
@Emilgardis Emilgardis mentioned this pull request Feb 16, 2024
Open
har7an added a commit to har7an/cross that referenced this pull request Feb 20, 2024
@har7an
docs/env_var: Document missing variable from cross-rs#661.
b9dfb2d
har7an added a commit to har7an/cross that referenced this pull request Feb 20, 2024
@har7an
docs/env_var: Document missing variable from cross-rs#661.
d82f223
This was referenced Nov 26, 2024
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alexhuszagh Alexhuszagh Alexhuszagh requested changes

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

Successfully merging this pull request may close these issues.

error/panic on fallback to host cargo or when in CI environment
2 participants
@Emilgardis @Alexhuszagh