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

fix path issue #994

Merged
merged 2 commits into from
Aug 30, 2022
Merged

fix path issue #994

merged 2 commits into from
Aug 30, 2022

Conversation

Emilgardis
Copy link
Member

@Emilgardis Emilgardis commented Aug 10, 2022 •
edited
Loading

fixes #993

+ /usr/local/bin/docker buildx build --label 'org.cross-rs.for-cross-target=aarch64-unknown-linux-gnu' --label 'org.cross-rs.runs-with=x86_64-unknown-linux-gnu' --label 'org.cross-rs.workspace_root=/tmp/tmp.cmePkb/workspace' --tag localhost/cross-rs/cross-custom-workspace:aarch64-unknown-linux-gnu-e570c-pre-build --build-arg 'CROSS_CMD=exit' --build-arg 'CROSS_DEB_ARCH=arm64' --file /tmp/tmp.cmePkb/workspace/target/aarch64-unknown-linux-gnu/Dockerfile.aarch64-unknown-linux-gnu-custom --output 'type=docker' /var/lib/docker/overlay2/5b7e7f3b1f39b622d2a665281973f5c1bdc2dce21f33c0bc105ceee253cfbe42/merged/tmp/tmp.cmePkb
error: unable to prepare context: path "/var/lib/docker/overlay2/5b7e7f3b1f39b622d2a665281973f5c1bdc2dce21f33c0bc105ceee253cfbe42/merged/tmp/tmp.cmePkb" not found
Error: 
   0: could not run container
   1: when building custom image
   2: when pre-building
   3: `/usr/local/bin/docker buildx build --label 'org.cross-rs.for-cross-target=aarch64-unknown-linux-gnu' --label 'org.cross-rs.runs-with=x86_64-unknown-linux-gnu' --label 'org.cross-rs.workspace_root=/tmp/tmp.cmePkb/workspace' --tag localhost/cross-rs/cross-custom-workspace:aarch64-unknown-linux-gnu-e570c-pre-build --build-arg 'CROSS_CMD=exit' --build-arg 'CROSS_DEB_ARCH=arm64' --file /tmp/tmp.cmePkb/workspace/target/aarch64-unknown-linux-gnu/Dockerfile.aarch64-unknown-linux-gnu-custom --output 'type=docker' /var/lib/docker/overlay2/5b7e7f3b1f39b622d2a665281973f5c1bdc2dce21f33c0bc105ceee253cfbe42/merged/tmp/tmp.cmePkb` failed with exit status: 1

this issue presents because the --file arg is wrong

@Emilgardis

This comment was marked as outdated.

Sign in to view
bors bot added a commit that referenced this pull request Aug 10, 2022
@bors
Try #994: --target x86_64-unknown-linux-gnu
1174b4f
@Emilgardis Emilgardis added the no changelog A valid PR without changelog (no-changelog) label Aug 10, 2022
@bors

This comment was marked as outdated.

Sign in to view
@Emilgardis

This comment was marked as outdated.

Sign in to view
bors bot added a commit that referenced this pull request Aug 10, 2022
@bors
Try #994: --target x86_64-unknown-linux-gnu
0cf3414
@bors

This comment was marked as outdated.

Sign in to view
@Emilgardis Emilgardis mentioned this pull request Aug 10, 2022
Closed
11 tasks
@Emilgardis Emilgardis linked an issue Aug 10, 2022 that may be closed by this pull request
when pre-building custom docker image inside docker container cross uses weird path #993
Closed
11 tasks
@Emilgardis

This comment was marked as outdated.

Sign in to view
bors bot added a commit that referenced this pull request Aug 10, 2022
@bors
Try #994: --target x86_64-unknown-linux-gnu
ac22a53
@bors

This comment was marked as outdated.

Sign in to view
@Emilgardis Emilgardis force-pushed the fix/issue993 branch 3 times, most recently from c593fa3 to caab4d4 Compare August 10, 2022 12:55
@Emilgardis

This comment was marked as outdated.

Sign in to view
bors bot added a commit that referenced this pull request Aug 10, 2022
@bors
Try #994: --target cross
9e1ee49
@bors

This comment was marked as outdated.

Sign in to view
@Emilgardis

This comment was marked as outdated.

Sign in to view
bors bot added a commit that referenced this pull request Aug 10, 2022
@bors
Try #994: --target cross
54018bb
@Emilgardis Emilgardis marked this pull request as ready for review August 10, 2022 13:26
@Emilgardis Emilgardis requested a review from a team as a code owner August 10, 2022 13:26
@Emilgardis Emilgardis removed the no changelog A valid PR without changelog (no-changelog) label Aug 10, 2022
@bors

This comment was marked as outdated.

Sign in to view
@Emilgardis Emilgardis force-pushed the fix/issue993 branch 2 times, most recently from 06e8f2c to 4dc5b55 Compare August 10, 2022 13:35
@Emilgardis
Copy link
Member Author

should fail, no fix applied

bors try --target cross

@Emilgardis

This comment was marked as outdated.

Sign in to view
bors bot added a commit that referenced this pull request Aug 10, 2022
@bors
Try #994: --target cross
7bc7ee9
@bors

This comment was marked as outdated.

Sign in to view
@Emilgardis
Copy link
Member Author

bors try --target cross

bors bot added a commit that referenced this pull request Aug 10, 2022
@bors
Try #994: --target cross
5cdc1b4
@bors

This comment was marked as outdated.

Sign in to view
@Emilgardis

This comment was marked as outdated.

Sign in to view
@bors

This comment was marked as off-topic.

Sign in to view
@Emilgardis
Copy link
Member Author

bors try-

bors try --target cross

bors bot added a commit that referenced this pull request Aug 10, 2022
@bors
Try #994: --target cross
497d533
@bors
Copy link
Contributor

bors bot commented Aug 10, 2022

try

Build failed:

@Emilgardis
Copy link
Member Author

it failed for the correct reason now!

bors try --target cross

bors bot added a commit that referenced this pull request Aug 10, 2022
@bors
Try #994: --target cross
80ac16c
@bors
Copy link
Contributor

bors bot commented Aug 10, 2022

try

Build succeeded:

Emilgardis
Emilgardis commented Aug 10, 2022
View reviewed changes
Copy link
Member Author

@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.

this should be good to go

ci/test-docker-in-docker.sh Show resolved Hide resolved
src/docker/shared.rs Outdated
@@ -261,7 +261,7 @@ pub struct Directories {
impl Directories {
pub fn create(
mount_finder: &MountFinder,
metadata: &CargoMetadata,
metadata: &mut CargoMetadata,
Copy link
Member Author

Choose a reason for hiding this comment

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

to ensure that further path references are correct, we specify this as &mut

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I like this design: we should probably either:

  1. Factor this out into 2 functions (one to find the mount path of the target directory and one for Directories::create)
  2. Rename the function so it's clear it modifies CargoMetadata.

src/docker/shared.rs Outdated
@@ -335,7 +335,7 @@ impl Directories {
Ok(Directories {
cargo,
xargo,
target,
target: metadata.target_directory.clone(),
Copy link
Member Author

Choose a reason for hiding this comment

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

ideally, we'd not have this here, but sometimes we use Directories, sometimes we use CargoMetadata, ideally we'd only use one CargoMetadata

@Emilgardis Emilgardis added this to the v0.3.0 milestone Aug 10, 2022
Alexhuszagh
Alexhuszagh requested changes Aug 10, 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.

Directories::create with the mount finder changes needs to be refactored into 2 functions, or the function needs to be renamed. Something that is supposed to create a new struct shouldn't modify one of its arguments.

src/docker/shared.rs Outdated
@@ -261,7 +261,7 @@ pub struct Directories {
impl Directories {
pub fn create(
mount_finder: &MountFinder,
metadata: &CargoMetadata,
metadata: &mut CargoMetadata,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I like this design: we should probably either:

  1. Factor this out into 2 functions (one to find the mount path of the target directory and one for Directories::create)
  2. Rename the function so it's clear it modifies CargoMetadata.

@Alexhuszagh
Copy link
Contributor

Nice change with create to assemble.

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 30, 2022

👎 Rejected by code reviews

Alexhuszagh
Alexhuszagh approved these changes Aug 30, 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.

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 30, 2022

Build succeeded:

@bors bors bot merged commit 47df5c7 into cross-rs:main Aug 30, 2022
@Alexhuszagh Alexhuszagh added bug 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

@Alexhuszagh Alexhuszagh Alexhuszagh approved these changes

Assignees
No one assigned
Labels
bug 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.

when pre-building custom docker image inside docker container cross uses weird path
2 participants
@Emilgardis @Alexhuszagh