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

Add dockerfile for cross. #878

Merged
merged 1 commit into from
Jul 5, 2022
Merged

Add dockerfile for cross. #878

merged 1 commit into from
Jul 5, 2022

Conversation

Alexhuszagh
Copy link
Contributor

@Alexhuszagh Alexhuszagh commented Jun 27, 2022

This uses the official Docker docker-in-docker image for the latest stable, and installs the latest stable Rust version internally. It uses an entrypoint to ensure cargo is on the path, and that cross is installed from the latest git (locked), which when tagged will work well with our releases. It exports the environment variable CROSS_CONTAINER_IN_CONTAINER so everything should work as expected.

Closes #468.
Replaces #667.

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jun 27, 2022

For some reason, the tags aren't working:
https://github.com/cross-rs/cross/runs/7082679487?check_suite_focus=true#step:5:13

I might have to override some of the envvars.

@Alexhuszagh
Copy link
Contributor Author

bors try --target cross

bors bot added a commit that referenced this pull request Jun 28, 2022
@Alexhuszagh

This comment was marked as outdated.

@bors

This comment was marked as outdated.

@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Jun 28, 2022
@bors

This comment was marked as outdated.

@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Jun 28, 2022
@Alexhuszagh

This comment was marked as outdated.

@bors

This comment was marked as outdated.

@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Jun 28, 2022
@bors

This comment was marked as outdated.

@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Jun 28, 2022
@bors

This comment was marked as outdated.

@Alexhuszagh
Copy link
Contributor Author

I managed to get this to work by effectively skipping the test if it was a Docker image for cross, but that's definitely not the solution here. Any suggestions would be great, since my idea was to skip testing if the matrix.target == 'cross', but I didn't have any luck with:

  • ${{ matrix.target != 'cross' }}
  • ${{ matrix.target }} != 'cross'

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.

Does this entry point approach not incur quite a bit of startup?

Also, this doesnt test the code in the branch or even local base, but current main

@Emilgardis
Copy link
Member

I managed to get this to work by effectively skipping the test if it was a Docker image for cross, but that's definitely not the solution here. Any suggestions would be great, since my idea was to skip testing if the matrix.target == 'cross', but I didn't have any luck with:

* `${{ matrix.target != 'cross' }}`

* `${{ matrix.target }} != 'cross'`

Instead of doing that, better approach I think would be to make something similar to

gha_output("has-image", "true")
, and gate the test on that

The first alternative is correct also, however in if statements you dont need ${{}}

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jun 28, 2022

Yeah I probably need another test script or something. Also, it's probably a good idea to move this to Ubuntu, since all our other images are. I think having a single Alpine image probably isn't worth it. It's also a much larger image since it needs a redundant x86_64-unknown-linux-musl as a result.

Maybe the integration tests for docker-in-docker could be based off of this?

@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Jun 28, 2022
@Alexhuszagh

This comment was marked as outdated.

@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Jun 30, 2022
@bors

This comment was marked as outdated.

CHANGELOG.md Outdated Show resolved Hide resolved
docker/cross.sh Outdated Show resolved Hide resolved
@Alexhuszagh Alexhuszagh force-pushed the dind branch 2 times, most recently from 53a737c to 953ea7f Compare July 1, 2022 01:39
@Alexhuszagh
Copy link
Contributor Author

I changed the Dockerfile context to the workspace root, and then use that in the build process, so we can copy over the entire project over from the current source code, to ensure the image reflects the current project state.

bors try --target cross

bors bot added a commit that referenced this pull request Jul 1, 2022
@bors

This comment was marked as outdated.

@Alexhuszagh Alexhuszagh force-pushed the dind branch 3 times, most recently from 41ca68c to a648e69 Compare July 1, 2022 14:31
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.

LGTM!

One thing missing is changing the label for title, it should probably be simply "cross" or similar

@Alexhuszagh
Copy link
Contributor Author

bors try --target cross

bors bot added a commit that referenced this pull request Jul 1, 2022
@bors
Copy link
Contributor

bors bot commented Jul 1, 2022

try

Build succeeded:

This uses the installs Docker from the official sources using the latest Ubuntu image stable, and installs the latest stable Rust version internally. Cross is installed from the latest git (locked), which when tagged will work well with our releases. It exports the environment variable `CROSS_CONTAINER_IN_CONTAINER` so everything should work as expected.
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.

lgtm!

@Alexhuszagh
Copy link
Contributor Author

bors r=Emilgardis

@bors
Copy link
Contributor

bors bot commented Jul 5, 2022

Build succeeded:

@bors bors bot merged commit 7331bb9 into cross-rs:main Jul 5, 2022
@Alexhuszagh Alexhuszagh deleted the dind branch July 5, 2022 18:42
@Emilgardis Emilgardis added this to the v0.2.3 milestone Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Images with cargo and rustc built-in?
2 participants