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.

Sign up for GitHub

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

Jump to bottom

Ruby setup #250

Merged
merged 6 commits into from
May 20, 2024
Merged

Ruby setup #250

merged 6 commits into from
May 20, 2024

Conversation

lsf37
Copy link
Member

@lsf37 lsf37 commented May 20, 2024 •
edited
Loading

  • update Gemfile.lock and bring in line with pinned Ruby version
  • set up correct Ruby version in CI
  • cache Ruby deps in CI
  • describe rbenv setup in README
  • reorg README to make install/build easier

@lsf37 lsf37 force-pushed the ruby-setup branch 2 times, most recently from 84a759d to 8fbe7df Compare May 20, 2024 01:19
@lsf37 lsf37 requested a review from Indanz May 20, 2024 01:26
@lsf37
Copy link
Member Author

lsf37 commented May 20, 2024

I'm not entirely sure why I had to downgrade sass-embedded from 1.54 to 1.53, but I got dependency conflicts for anything > 1.53 and it's not like we're using this on untrusted input or are currently hitting bugs. Longer term it'd be nice to upgrade the Ruby version, but that's more time than I'm willing to spend today.

@lsf37
Copy link
Member Author

lsf37 commented May 20, 2024

Tested with:

  • Darwin Intel, Darwin Apple Silicon
  • Linux x64 and arm64 via Docker

Indanz
Indanz approved these changes May 20, 2024
View reviewed changes
Copy link
Contributor

@Indanz Indanz left a comment

Choose a reason for hiding this comment

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

My main concern is that you seem to be manually editing Gemfile.lock. You should never do that, all changes should come via requests in Gemfile or bundler update or similar.

The other changes seem fine.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@lsf37
Copy link
Member Author

lsf37 commented May 20, 2024

My main concern is that you seem to be manually editing Gemfile.lock. You should never do that, all changes should come via requests in Gemfile or bundler update or similar.

The commit is the result of bundle update, with some versions constrained in Gemfile (in the same commit) and the Ruby version (and therefore initial bundler version) pinned in .ruby-version. This should be fully stable.

lsf37 added 6 commits May 21, 2024 09:32
@lsf37
update Gemfile.lock, constrain sass-embedded
2bc4411 
- Constrain sass-embedded to <=1.53 in Gemfile, because later versions
  produce version conflicts on Linux.
- Update locked versions with `bundle upate`` and bring bundler version
  in line with pinned ruby version.

Signed-off-by: Gerwin Klein <[email protected]>
@lsf37
github: set up .ruby-version + cache dependencies
89007d1 
- set up Ruby according to .ruby-version
- cache Ruby deps

Signed-off-by: Gerwin Klein <[email protected]>
@lsf37
reorganise README, add Ruby instructions
00ace8a 
- reorganise README to show build instructions earlier
- seprate install from build
- rewrap at 80

Signed-off-by: Gerwin Klein <[email protected]>
@lsf37
README: add python install section
3af332b 
This should really be managed with a requirements.txt and some kind of
virtual env setup, but that's a later step.

Signed-off-by: Gerwin Klein <[email protected]>
@lsf37
Gemfiles.lock: add supported platforms
c3c8d0d 
Add missing platforms with `bundle lock --add-platform ..` and
`bundle update`.

Not tested on Windows, but the tzinfo dependency was added specifically
for Windows at some point, so it may well work. Adding the Windows
platform removes the annoying warning about tzinfo.

Signed-off-by: Gerwin Klein <[email protected]>
@lsf37
README: bundle install not necessary
d90d1ae 
The Makefile already runs `bundle install`, so we don't need to call it
manually.

Signed-off-by: Gerwin Klein <[email protected]>
@lsf37
Copy link
Member Author

lsf37 commented May 20, 2024

Rebased, addressed review feedback and documented in the commit message how Gemfile.lock was updated for the future.

@lsf37
Copy link
Member Author

lsf37 commented May 20, 2024

That dependency caching for Ruby is quite nice. The build is much faster now.

@lsf37 lsf37 merged commit 4a75e42 into master May 20, 2024
8 checks passed
@lsf37 lsf37 deleted the ruby-setup branch May 20, 2024 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Indanz Indanz Indanz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lsf37 @Indanz