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 pre-build hooks #635

Closed
wants to merge 1 commit into from
Closed

Conversation

aig787
Copy link

@aig787 aig787 commented Feb 7, 2022

This PR addresses #565 and hopefully a number of the open issues about the lack of openssl in the builders. It does add a dependency on gosu which handles lowering the privileges down to user level after the pre-build hooks are run. For now I've only added it to the x86_64-unknown-linux-gnu target for testing pending feedback on it.

I was able to build Cargo successfully with all features with

[target.x86_64-unknown-linux-gnu]
image = "rustembedded/cross:x86_64-unknown-linux-gnu-0.2.1"
pre-build = """
yum update -y
yum install -y openssl-devel
"""

@aig787 aig787 requested a review from a team as a code owner February 7, 2022 04:18
@Emilgardis
Copy link
Member

Awesome!

This is really a workaround but it's quite neat.
Why do we need to lower privileges?

@aig787
Copy link
Author

aig787 commented Feb 7, 2022

I'm not sure we necessarily need to - just seemed good practice to me especially since when hooks aren't provided the containers are run with the --user argument. Lowering privileges was just an attempt at getting us into the same state for the build whether they were provided or not.

Copy link
Contributor

@otavio otavio left a comment

Choose a reason for hiding this comment

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

I just small code comment, but I'd like to say I like the idea, and it does avoid the need for many custom images.

Comment on lines +195 to +207
// We need to specify the user for Docker, but not for Podman.
let uid_gid = if let Ok(ce) = get_container_engine() {
if ce.ends_with(DOCKER) {
Some((
env::var("CROSS_CONTAINER_UID").unwrap_or(id::user().to_string()),
env::var("CROSS_CONTAINER_GID").unwrap_or(id::group().to_string()),
))
} else {
None
}
} else {
None
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move the code back to the original place as it is a non-required code change.

Copy link
Author

@aig787 aig787 Mar 17, 2022

Choose a reason for hiding this comment

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

I can revert some of this, but not all of it. In order for for the pre-build hooks to install packages we have to remove the --user flag when hooks are present. So we can either

  • Remove --user all the time (cargo executes as root)
  • Check for pre-build hooks and remove --user when they exist (cargo executes as root only when pre-build hooks are specified)
  • Check for pre-build hooks and remove --user when they exist but de-escalate privileges so cargo still runs with the specified UID/GID (what the PR currently does)

I don't have a strong opinion which of them to use - but whichever one we go with this particular block has to move.

@Emilgardis Emilgardis marked this pull request as draft March 17, 2022 16:31
@Emilgardis
Copy link
Member

marking as draft as only one target is implemented right now

@aig787
Copy link
Author

aig787 commented Mar 31, 2022

marking as draft as only one target is implemented right now

I'm happy to get moving on the other targets if we're happy with the implementation. I'm also happy to revert the privilege de escalation first, just need to know what the preference is from your side.

@Emilgardis
Copy link
Member

marking as draft as only one target is implemented right now

I'm happy to get moving on the other targets if we're happy with the implementation. I'm also happy to revert the privilege de escalation first, just need to know what the preference is from your side.

I think the gid/uid setting is good, there is a reason we allow setting it.

@Emilgardis
Copy link
Member

see my comment on #643 (comment) regarding the "issue" that the image is now centos.

@Emilgardis
Copy link
Member

Emilgardis commented Apr 5, 2022

Been thinking about this some more now, would it make sense to actually not do this, and just use the technique in #678 and hook into a dockerfile passed to stdin to docker build --build-arg cmd=blabla [additional flags] -

FROM {target}
ARG CMD=exit 0
RUN $CMD

This way we can even support custom images, and we get a cache

@Alexhuszagh Alexhuszagh added enhancement S-waiting-on-author Status: Author needs to make changes and removed S-waiting-on-author Status: Author needs to make changes labels May 29, 2022
@patrickelectric
Copy link

This is a great feature!

@Alexhuszagh
Copy link
Contributor

Closed since it has been superseded by #678.

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.

5 participants