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

Improve building experience for packagers #421

Merged
merged 2 commits into from
Jun 19, 2023

Conversation

fracek
Copy link
Contributor

@fracek fracek commented Jun 16, 2023

Hello,

I'm building a nix overlay for Cairo that also builds scarb. I noticed a couple of issues while packaging scarb, this PR fixes them.

  • Rust version is very recent (1.70.0) but it's actually not required. The project builds fine with older rust versions (up to 1.64).
  • The Cairo archive is downloaded as part of the build process. This is an issue because sometimes build servers cut internet access off to ensure builds are reproducible. A common strategy adopted by other packages that need to download archives as part of the build process is to allow passing an already downloaded archive through an environment variable.

@fracek fracek requested a review from mkaput as a code owner June 16, 2023 18:57
Copy link
Contributor

@maciektr maciektr left a comment

Choose a reason for hiding this comment

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

Great work on the nix overlay! 🔥 🔥 🔥

Thanks for posting your changes here.
I've left one nitpick below.

scarb/build.rs Show resolved Hide resolved
@fracek
Copy link
Contributor Author

fracek commented Jun 17, 2023

Sorry I missed your comment. I will fix it tomorrow and update the pr.

@fracek fracek force-pushed the build-fix branch 2 times, most recently from 6397a96 to 66579ad Compare June 18, 2023 19:32
@maciektr maciektr added this pull request to the merge queue Jun 19, 2023
Merged via the queue into software-mansion:main with commit b10e5b9 Jun 19, 2023
@mkaput
Copy link
Member

mkaput commented Jun 26, 2023

@fracek Hey! I'm back from being offline and I wanted to actually elaborate on this.

The Cairo archive is downloaded as part of the build process. This is an issue because sometimes build servers cut internet access off to ensure builds are reproducible. A common strategy adopted by other packages that need to download archives as part of the build process is to allow passing an already downloaded archive through an environment variable.

That's great part

Rust version is very recent (1.70.0) but it's actually not required. The project builds fine with older rust versions (up to 1.64).

This is something I have mixed feelings about. Although indeed we build successfully on 1.64, we do not want to guarantee it. We definitely want to have a possibility to use newer Rust features (std APIs etc.). That's why I kept our MRSV (minimum Rust version) to latest stable - to publicly announce it, though indeed I didn't communicate it.

I would like to revert this part of this PR if possible, but beforehand we have to align ourselves, and thus my question is: why cannot you use latest Rust stable in your environment?

@fracek
Copy link
Contributor Author

fracek commented Jun 26, 2023

Allowing an older version of the compiler makes the life of the packager easier because some distributions are slower at updating cargo.

In my case it means I don't have to require users to use nix unstable and they can use the latest tagged release (23.05).
Similarly debian is at cargo 1.66 (same for ubuntu, ubuntu 22.04 LTS is at 1.58), so by sticking to cargo 1.64+ someone in the future can easily make a ppa for ubuntu.

@mkaput
Copy link
Member

mkaput commented Jun 28, 2023

Hmm, so do I understand correctly that you are building Scarb yourself? I don't think that's gonna be a good idea long term, because we plan to keep an organised directory structure of Scarb installation and allow us to keep some assets in future releases. We have our build scripts for this reason, and we'd like to reserve us a space for doing changes here or assumptions and so...

I wonder if it would make sense for you to just pull binaries built by us. Like, treat Scarb as closed-source software where you only have binaries at hand here. Would that work for you? This way we could still use whatever Rust toolchain we want without interrupting you.

@fracek
Copy link
Contributor Author

fracek commented Jun 28, 2023

As a general rule, distributions don't allow to pull binaries especially if it's possible to build from source (it's not safe to download binaries from the internet). Part of the packager's job is to understand the package and adapt it to the target distribution, so if you have any specific asset folders then they would place them it the correct directory (super easy if you follow the xdg convention).

My change to the cargo version was mostly to simplify the life for someone building scarb for ubuntu and other lts distros, obviously it's fine if you bump the minimum required version to 1.70 when you need it.

@mkaput
Copy link
Member

mkaput commented Jun 29, 2023

As a general rule, distributions don't allow to pull binaries especially if it's possible to build from source (it's not safe to download binaries from the internet). Part of the packager's job is to understand the package and adapt it to the target distribution, so if you have any specific asset folders then they would place them it the correct directory (super easy if you follow the xdg convention).

That's fine, but we also may end up writing new things, chmod +xing them etc with #188, that are just a vague idea at this moment and the horizon is several months from now on still. I'd like to highlight this to you. That's one of the reasons why the installer script puts files in $HOME/.local, why we work on the ASDF plugin, and why we don't publish DEBs and RPMs ourselves.

My change to the cargo version was mostly to simplify the life for someone building scarb for ubuntu and other lts distros, obviously it's fine if you bump the minimum required version to 1.70 when you need it.

OK, if doing this is fine for you, then it's great for us :) Thank you!

@fracek
Copy link
Contributor Author

fracek commented Jun 29, 2023

That's fine, but we also may end up writing new things, chmod +xing them etc with #188, that are just a vague idea at this moment and the horizon is several months from now on still. I'd like to highlight this to you. That's one of the reasons why the installer script puts files in $HOME/.local, why we work on the ASDF plugin, and why we don't publish DEBs and RPMs ourselves.

I think that's fine since it's after installation.You should always do things in a way that makes sense for scarb and if they pose an issue for packaging I'll open an issue and propose an alternative solution.

@mkaput
Copy link
Member

mkaput commented Jun 29, 2023

Perfect! Thank you again for explaining all of this :)

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

Successfully merging this pull request may close these issues.

3 participants