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 Windows to platforms for oj and yajl #216

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Apr 7, 2024

I forgot that "ruby" doesn't include MS Windows in the Gemfile platforms. Since it's reasonable to expect that a compiler is available on Windows these days, this should be added.

I was able to build and test this on my Windows 7 VM successfully.

In order to verify this worked I had to update the github action config, and did some refactoring while I was here.

@djberg96
Copy link
Contributor Author

djberg96 commented Apr 7, 2024

I had to update the bundler version so that it understood the "windows" platform. However, github actions has apparently decided to gaslight me. No matter what version of json or json_pure I specify, it says it can only find a slightly older version. So, if I tell it to use json_pure, "~> 2.7.2" it will say it can only find 2.6.x. If I specify 2.6.x it says it can only find 2.5.x, and so on.

I have no clue wtf it's doing.

Update: Nevermind, fixed it.


on:
push:
branches: [main, master] # TODO: rename and get rid of 'master'
# Triggers the workflow on pull request events.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to specify "opened, reopened, synchronize", those are the defaults.

concurrency:
group: "${{ github.workflow }} @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}"
cancel-in-progress: true

Copy link
Contributor Author

@djberg96 djberg96 Apr 8, 2024

Choose a reason for hiding this comment

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

Concurrency? For a small library like this? Nah. Let's simplify.

ruby-version: ['3.0', '3.1', '3.2', 'jruby']
platform: [ubuntu-latest, macos-latest, windows-latest]
runs-on: ${{ matrix.platform }}

Copy link
Contributor Author

@djberg96 djberg96 Apr 8, 2024

Choose a reason for hiding this comment

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

I refactored this, easier to read. This is basically the pattern I use for all of my gems.

I also added macos and windows to the platform list, and got rid of the SKIP_ADAPTERS env since you're not using it any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should also mention that I left truffleruby off for now because it smacked into some core language bug with symbol counts.

gem install bundler
bundle install
bundle exec rake

Copy link
Contributor Author

@djberg96 djberg96 Apr 8, 2024

Choose a reason for hiding this comment

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

I had to refactor this a bit because older versions of Ruby default to a version of Bundler that doesn't understand the "windows" platform for the Gemfile. Thus, the bundler-cache option is disabled, and an explicit gem install bundler step is added.

# Allows you to run this workflow manually from the Actions tab
branches: [main, master] # TODO: rename and get rid of 'master'
paths-ignore:
- '**/*.md'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor refactor, you don't want CI running for doc changes do you? Of course not.

gem "oj", "~> 3.0", platforms: [:ruby], require: false
gem "yajl-ruby", "~> 1.3", platforms: [:ruby], require: false
gem "oj", "~> 3.0", platforms: [:ruby, :windows], require: false
gem "yajl-ruby", "~> 1.3", platforms: [:ruby, :windows], require: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing that started it all.

@djberg96
Copy link
Contributor Author

djberg96 commented Apr 8, 2024

@sferik Ok, ready for review. :)

@djberg96
Copy link
Contributor Author

@sferik Thoughts?

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.

1 participant