-
Notifications
You must be signed in to change notification settings - Fork 102
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 GitHub Actions - Ubuntu, macOS, & Windows, Ruby 2.4 thru head, misc updates #83
Add GitHub Actions - Ubuntu, macOS, & Windows, Ruby 2.4 thru head, misc updates #83
Conversation
0b1260e
to
62274f7
Compare
Sorry for the ping. I forgot that I was a collaborator here. I am busy enough with other OSS, and I don't know Textile docs. So, I'm not sure if I can help with real code issues here, but the infrastructure things (build scripts, CI, etc) I can help with. I think #74, #82, and this PR can be merged, and they'll give a good start to updating things, along with CI. I haven't worked with any CI going back before Ruby 2.4 for a while. I know Windows is really a PITA, not sure about macOS or Ubuntu. I did not add a So, wondering if you have any opinions... |
62274f7
to
915f1b7
Compare
Added Ruby 2.2 & 2.3 on Ubuntu & macOS. All four jobs passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MSP-Greg : thanks again for sending this PR.
Please take a look at my comments.
@@ -1,81 +0,0 @@ | |||
require 'rvm' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MSP-Greg : can you please revert this change?
I still used this one when I released the last version of the gem.
I want to do a clean up as well, but not in the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the current file, which was last updated in Apr-2016...
RVM_RUBIES = ['ruby-1.8.6-p398', 'ruby-1.9.1-p243', 'ruby-1.9.2-p136', 'ruby-2.2.3p173']
I still used this one when I released the last version of the gem.
For testing? That's what CI is for. BTW, releases, maybe a sensible required_ruby_version
in the gemspec? I'd suggest 2.4...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MSP-Greg : that is not for testing.
Is this change just for cleanup? Or this breaks your windows setup as well?
New workflow files never run in upstream from a PR. They will run in forks, because they're normally running from a branch. Rebased and added Ruby 3.3. to the workflow. I'll wait until #62 or #82 are merged, as CI will fail until then.
Yes, and I've needed to patch that every time I install RedCloth on my Windows system. I have a lot of Rubies installed on all my systems. Both #62 and #82 are simple PR's. #62 did not delete the code related to 'pre-compiled' gems, #82 did. |
@MSP-Greg : I've merged #62 , can you please check if that suffice for this PR? I spent some time reading your comments in different issues to situate myself on all the windows issues you've had in the past years (way before I even knew this gem existed). This comment caught my eye: #51 (comment) It does look like there is a better way to do these extensions and nokogiri is a really good reference. Thank you for pointing that up, I will try to spend some time studying nokogiri's code. As I've mentioned in a few other comments I am still trying to situate myself so I don't break anything... :-D |
I think the common way of doing this is with https://github.com/rake-compiler/rake-compiler. |
|
b28315f
to
1b57559
Compare
I just rebased, added Ruby 3.3, and made use of a new feature in setup-ruby. All passed, see https://github.com/MSP-Greg/redcloth/actions/runs/7590837155/job/20678021321 Obviously, it takes longer than the current CI, but it tests all platforms. Note there are some warnings thrown in the compile step... |
Thank you @MSP-Greg . I will look further in the gem release process and perhaps work on a clean up PR that I've been planning. |
…dows Co-authored-by: Mohit Sindhwani <mohits@onghu.com>
Co-authored-by: Anton Maminov <anton.linux@gmail.com>
8415f67
to
9102f91
Compare
See https://github.com/MSP-Greg/redcloth/actions/runs/7121117282
Some of the changes are 'general cleanup', some are Windows specific changes.
I believe this needs PR #82 merged for Windows CI to pass.
closes #63, closes #71, closes #81