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

lock_platform directive in Gemfile #7172

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

martinemde
Copy link
Member

@martinemde martinemde commented Nov 17, 2023

What was the end-user or developer problem that led to this PR?

There are two problems I aim to solve:

1. First deploys can fail because the platform is not added

When a new user creates an application from a generator or template, they get a Gemfile that does not make it easy to see or declare the platform they use for deployment. It's not currently possible to add default platforms without additional steps. Deployment platform is not obvious, not immediately visible and not declarative within the most obvious place to declare dependencies, the Gemfile.

Many options have been proposed, including always locking linux or locking available platforms which have resulted in at least one problem.

This is notably different than a previous attempt, way back in 2016, to add a similar DSL (#4895). This proposed option is purely additive and does not restrict the user to only installing on certain platforms. It simply ensures that a given platform is always present. The original solution 7 years ago resulted in force ruby platform.

My assumptions are as follows:

  • Most people deploy to x86_64-linux or know when they are not.
  • Most people will look at their Gemfile but almost never look at their whole Gemfile.lock
  • People not deploying to x86_64-linux would recognize and correct the platform if it is at the top of their Gemfile, but will mostly not notice when we automatically add a platform to their Gemfile.lock.

2. The deployment platform is important, and yet it is ephemeral

You could theoretically completely regenerate your Gemfile.lock by deleting and reinstalling (generating a fully new resolution), and it might work. However, you would always lose your locked platforms because they are not specified anywhere outside of the generated lockfile. Although this sort of regeneration is not recommended, it points out that locked platforms are ephemeral.

This is fine for most platforms. It's ok if we auto-add different darwin versions when a developer installs, but the deployment platform should be added since it will be auto-added, or it will fail on frozen installs. Since it is a strict deployment dependency that we auto-add if it's missing, it should be intentionally declared in the Gemfile.

Additionally, when we lock to checksums this will make it possible to lock checksums for the deployment platform more successfully, instead of allowing production to install without checksum verification or to fail when a frozen bundle is used and the deployment platform is not locked.

What is your fix for the problem, implemented in this PR?

Let's cut to the chase here. This is part of rails new:

         run  bundle install
Fetching gem metadata from https://rubygems.org/...........
Resolving dependencies...
Fetching public_suffix 5.0.4
Fetching nokogiri 1.15.5 (arm64-darwin)
Fetching nio4r 2.6.0
Installing public_suffix 5.0.4
Installing nio4r 2.6.0 with native extensions
Installing nokogiri 1.15.5 (arm64-darwin)
Fetching loofah 2.22.0
Installing loofah 2.22.0
Bundle complete! 14 Gemfile dependencies, 82 gems now installed.
Use `bundle info [gemname]` to see where a bundled gem is installed.
         run  bundle lock --add-platform=x86_64-linux
Fetching gem metadata from https://rubygems.org/.........
Resolving dependencies...
Writing lockfile to /Users/martinemde/Projects/tmp/rails712/Gemfile.lock
         run  bundle lock --add-platform=aarch64-linux
Fetching gem metadata from https://rubygems.org/.........
Resolving dependencies...
Writing lockfile to /Users/martinemde/Projects/tmp/rails712/Gemfile.lock

This causes 3 resolves and adds two additional platforms by default.

Let's allow rails to generate the following Gemfile and run only one resolve:

source "https://rubygems.org"

# Update this with your production deployment platforms
lock_platforms "x86_64-linux", "aarch64-linux"

ruby "3.2.2"

# Bundle edge Rails instead: gem "rails", github: "rails/rails", branch: "main"
gem "rails", "~> 7.1.2"
# SNIP...

Since Gemfiles are often generated or cargo culted from other Gemfiles, adding an option for specifying locked platforms in the Gemfile makes it more likely that initial deploys will succeed and already have the platformed gems for the deployment platform.

This PR adds a new Gemfile directive, lock_platform, that may be included by default in generated Gemfiles or added to declare and ensure the deployment platform (or other desired platforms).

This pull request is currently a draft to show a proof of concept and not all tangential cases have been considered. Please call out any problems you see that may result from this approach.

Make sure the following tasks are checked

bundler/lib/bundler/dsl.rb Outdated Show resolved Hide resolved
@@ -89,6 +89,7 @@ def initialize(lockfile, dependencies, sources, unlock, ruby_version = nil, opti
@locked_gems = LockfileParser.new(@lockfile_contents)
@locked_platforms = @locked_gems.platforms
@platforms = @locked_platforms.dup
platforms.each {|p| add_platform(p) }
Copy link
Member

Choose a reason for hiding this comment

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

does it work to call platforms.each {|p| add_platform(p) } unconditionally, at line 118?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably. This was a premature optimization when we know that @locked_platforms is empty when there's no lockfile.

@deivid-rodriguez
Copy link
Member

Many options have been proposed, including #5700 or locking available platforms which have resulted in #7159.

Just noticing that I already have a local fix for this. I don't think a bug should "drive" this discussion. I certainly expected some issues to come up from that change.

@martinemde
Copy link
Member Author

martinemde commented Nov 17, 2023

Just noticing that I already have a local fix for this. I don't think a bug should "drive" this discussion. I certainly expected some issues to come up from that change.

I agree. Let's not let this drive the discussion, but maybe we can decide if this is a preferred (edit: acceptable, additional) solution for the problems we see related to locked platforms.

@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Nov 17, 2023

I don't think these are mutually exclusive! I like being able to set target platforms for the lockfile, but I also like that Gemfiles are usable in many platforms by default. Locking multiple platforms seems to be a trend across several package managers.

@martinemde martinemde force-pushed the martinemde/gemfile-lock-platform-dsl branch from cf3f10d to aa17886 Compare November 17, 2023 18:56
@indirect
Copy link
Member

This seems reasonable to me! I am hoping that most of the current reasons to use this will be removed by universal lockfiles (🤞🏻) but this seems like a good option for the other cases.

@martinemde
Copy link
Member Author

martinemde commented Nov 19, 2023

#7169 might just be the specific case that benefits from this feature. I think right now resolving might fail if the gem is unavailable on a locked platform. It seems like a good test case if we can get this to support their needs.

@martinemde
Copy link
Member Author

Updated to prevent the lock_platforms from being removed even if they would be invalid normally, ensuring that an error is caused rather than produce a Gemfile.lock that cannot be deployed to a locked platform.

This allows the Gemfile to include a deploy platform by default on first
deploy, and since Gemfiles are often cargo culted or generated, we can
make the deploy platform intention more more clear from the beginning.
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.

None yet

4 participants