Skip to content

Conversation

Emilgardis
Copy link
Member

No description provided.

@Emilgardis Emilgardis requested a review from a team as a code owner June 30, 2022 09:05
@Emilgardis
Copy link
Member Author

Emilgardis commented Jun 30, 2022

this is targeted towards #273.

@schrieveslaach could you install this fork/rev and try if it spits out a more helpful error message. The binary will be available in artifacts soon.

bors try --target x86_64-unknown-linux-gnu

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

bors bot commented Jun 30, 2022

try

Build succeeded:

@Emilgardis Emilgardis added no changelog A valid PR without changelog (no-changelog) labels Jun 30, 2022
@Emilgardis
Copy link
Member Author

Emilgardis commented Jun 30, 2022

schrieveslaach added a commit to schrieveslaach/nextcloud-spgverein-app that referenced this pull request Jun 30, 2022
schrieveslaach added a commit to schrieveslaach/nextcloud-spgverein-app that referenced this pull request Jun 30, 2022
@schrieveslaach
Copy link

@Emilgardis, here are the results.

@Emilgardis
Copy link
Member Author

Awesome, thats a huge help, that file seems to be a symlink, so we're basically not handling that correctly

@Alexhuszagh
Copy link
Contributor

Alexhuszagh commented Jun 30, 2022

Awesome, thats a huge help, that file seems to be a symlink, so we're basically not handling that correctly

Ah I definitely didn't handle them for copying the toolchains, it might have slipped through to ordinary files. It should handle copying entire directories which contain symlinks fine, however.

@Alexhuszagh
Copy link
Contributor

We should probably also add a warning if generating the symlinks fails, like if this subcommand fails. It should probably say something like .wrap_err("when creating symlinks to provide consistent host/mount paths");

cross/src/docker/remote.rs

Lines 955 to 982 in e583132

symlink.push(format!(
"prefix=\"{mount_prefix}\"
symlink_recurse() {{
for f in \"${{1}}\"/*; do
dst=${{f#\"$prefix\"}}
if [ -f \"${{dst}}\" ]; then
echo \"invalid: got unexpected file at ${{dst}}\" 1>&2
exit 1
elif [ -d \"${{dst}}\" ]; then
symlink_recurse \"${{f}}\"
else
ln -s \"${{f}}\" \"${{dst}}\"
fi
done
}}
symlink_recurse \"${{prefix}}\"
"
));
for (src, dst) in to_symlink {
symlink.push(format!("ln -s \"{}\" \"{}\"", src.as_posix()?, dst));
}
subcommand(engine, "exec")
.arg(&container)
.args(&["sh", "-c", &symlink.join("\n")])
.run_and_get_status(msg_info, false)
.map_err::<eyre::ErrReport, _>(Into::into)?;

@Emilgardis
Copy link
Member Author

Emilgardis commented Jun 30, 2022

It should handle copying entire directories which contain symlinks fine, however.

The issue is this I believe.

cross/src/docker/remote.rs

Lines 247 to 254 in e583132

let src_path = file.path();
let dst_path = dst.join(file.file_name());
if file.file_type()?.is_file() {
fs::copy(&src_path, &dst_path)?;
} else {
fs::create_dir(&dst_path).ok();
copy_dir(&src_path, &dst_path, depth + 1, skip)?;
}

is_file == false for symlinks

@Alexhuszagh
Copy link
Contributor

Alexhuszagh commented Jun 30, 2022

is_file == false for symlinks

That should be it. I'll submit a fix shortly. I did that intentionally for toolchains, but I also mistakenly applied that for projects and other mounted volumes. EDIT: Implemented in #885.

@Emilgardis Emilgardis marked this pull request as draft June 30, 2022 14:22
@Alexhuszagh Alexhuszagh added this to the v0.2.3 milestone Jun 30, 2022
@Emilgardis Emilgardis marked this pull request as ready for review June 30, 2022 20:11
@Emilgardis Emilgardis force-pushed the more-verbose branch 2 times, most recently from 8fcf39a to c4afe6f Compare June 30, 2022 20:16
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.

Once this is implemented, it's good to go:

We should probably also add a warning if generating the symlinks fails, like if this subcommand fails. It should probably say something like .wrap_err("when creating symlinks to provide consistent host/mount paths");

cross/src/docker/remote.rs

Lines 955 to 982 in e583132

symlink.push(format!(
"prefix=\"{mount_prefix}\"
symlink_recurse() {{
for f in \"${{1}}\"/*; do
dst=${{f#\"$prefix\"}}
if [ -f \"${{dst}}\" ]; then
echo \"invalid: got unexpected file at ${{dst}}\" 1>&2
exit 1
elif [ -d \"${{dst}}\" ]; then
symlink_recurse \"${{f}}\"
else
ln -s \"${{f}}\" \"${{dst}}\"
fi
done
}}
symlink_recurse \"${{prefix}}\"
"
));
for (src, dst) in to_symlink {
symlink.push(format!("ln -s \"{}\" \"{}\"", src.as_posix()?, dst));
}
subcommand(engine, "exec")
.arg(&container)
.args(&["sh", "-c", &symlink.join("\n")])
.run_and_get_status(msg_info, false)
.map_err::<eyre::ErrReport, _>(Into::into)?;

@Emilgardis Emilgardis changed the title add some more context to error messages on remote::run add some more context to error messages Jun 30, 2022
@Alexhuszagh
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 30, 2022

Build succeeded:

@bors bors bot merged commit f059d1a into cross-rs:main Jun 30, 2022
@Emilgardis Emilgardis deleted the more-verbose branch June 30, 2022 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog A valid PR without changelog (no-changelog)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants