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

Implement pre-build copy #861

Closed
2 tasks done
Emilgardis opened this issue Jun 25, 2022 · 6 comments
Closed
2 tasks done

Implement pre-build copy #861

Emilgardis opened this issue Jun 25, 2022 · 6 comments
Assignees
Milestone

Comments

@Emilgardis
Copy link
Member

Checklist

Describe your request

From reddit

Make it easy to integrate scripts to the pre-build hook.

Describe why this would be a good inclusion for cross

No response

@Alexhuszagh
Copy link
Contributor

Alexhuszagh commented Jun 25, 2022

I'm not sure this is needed: I remember being initially very favorable of the idea of supporting other commands in the pre-build hooks or customizing the base image, but in retrospect it seems like the solution we implemented was much better.

Currently, we have:

[target.x86_64-unknown-linux-gnu]
pre-build = [
    "dpkg --add-architecture arm64", 
    "apt-get update && apt-get install --assume-yes libssl-dev:arm64"
]

And my initial proposal would have been:

[target.x86_64-unknown-linux-gnu]
pre-build = [
    "RUN dpkg --add-architecture arm64", 
    "RUN apt-get update && apt-get install --assume-yes libssl-dev:arm64"
]

Which honestly in retrospect is not very legible, and a Dockerfile is better in that case. If we use a copy, will all copies be done before all runs? How will this affect cached builds if another step is added? What else do we need to implement? ENV? ARG? I think just using a Dockerfile or simple pre-build RUN instructions is enough.

The current solution for a Dockerfile is just much nicer for this:

FROM ghcr.io/cross-rs/x86_64-unknown-linux-gnu:main
RUN dpkg --add-architecture arm64
RUN apt-get update && apt-get install --assume-yes libssl-dev:arm64

COPY myscript.sh /
RUN /myscript.sh

@Emilgardis
Copy link
Member Author

We only have one RUN inserted, each value in pre-build is separated by a newline and evaluated with eval

I think it'd be fine to include a way to call a script specifically, maybe that usecase is what should be supported and not a general "copy"

@Alexhuszagh
Copy link
Contributor

We only have one RUN inserted, each value in pre-build is separated by a newline and evaluated with eval

I think it'd be fine to include a way to call a script specifically, maybe that usecase is what should be supported and not a general "copy"

I like that better, and make it clear it would happen after the pre-build run commands, which I believe is best.

@Emilgardis
Copy link
Member Author

maybe simply

[*]
pre-build = "./myscript.sh" # Note: not an array.

don't think we need to support pre-build via array and this new alternative

@Alexhuszagh
Copy link
Contributor

Alexhuszagh commented Jun 26, 2022

That makes it super simple, and correct. I think that's the correct approach. We should probably do:

  • Allow an array or string
  • If a string, check if it's a valid file, in which case copy and run it.
  • If a string but not a valid file, treat it as a single-element array.
  • Convert array to run commands, convert string to COPY + RUN.

@Alexhuszagh Alexhuszagh added this to the v0.2.3 milestone Jun 26, 2022
bors bot added a commit that referenced this issue Jul 8, 2022
910: add way to run script with pre-build r=Alexhuszagh a=Emilgardis

Solves #861


Co-authored-by: Emil Gardström <emil.gardstrom@gmail.com>
@Alexhuszagh
Copy link
Contributor

Alexhuszagh commented Jul 9, 2022

This was implemented in #910.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants