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

Fixes loading problems when Gem is built for Windows #62

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

mohits
Copy link
Contributor

@mohits mohits commented Mar 2, 2020

On Windows, the loading code in redcloth.rb assumed that one is loading it from a 'fat binary' gem. Using standard windows Ruby versions from 2.4 forward, the gem can be compiled by the user.

This change first tries to require the library using the prefix (for fat gems). If that fails, it tries again to require without the prefix (for built gems).

References: #51 and #61 for Windows users.

mohits added 2 commits March 2, 2020 22:26
On Windows, the loading code in redcloth.rb assumed that one is loading it from a 'fat binary' gem. Using standard windows Ruby versions from 2.4 forward, the gem can be compiled by the user.

This change first tries to require the library using the prefix (for fat gems). If that fails, it tries again to require without the prefix (for nuilt gems).

Fixes jgarber#51 and jgarber#61.
Fix loading problems when Gem is built on Windows
@mohits
Copy link
Contributor Author

mohits commented Apr 21, 2022

Thanks @Silverfishnet! I have been wanting to get back to this gem to fix the JRuby issues... and this action of yours is motivation to do it now.

@@ -10,6 +10,11 @@
conf = Object.const_get(defined?(RbConfig) ? :RbConfig : :Config)::CONFIG
prefix = conf['arch'] =~ /mswin|mingw/ ? "#{conf['MAJOR']}.#{conf['MINOR']}/" : ''
lib = "#{prefix}redcloth_scan"
begin
Copy link
Collaborator

Choose a reason for hiding this comment

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

@mohits : there is another suggestion for this fix in #82 .

I am looking into the current PRs open and trying to merge them all.
Please let me know if you have a bit of time to review it for possible issues.

I commented in there as well that I find your code more safer, given I am still situating myself, and still moving slower.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @heliocola - I put this gem down in the list of things that I want to get back to and work on this year... just didn't expect it to fire up this early in the year. Let me try to put things back together and try to review the requirements again, and where I had stopped.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you @mohits !

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mohits : FYI: I will merge this one as it helps @MSP-Greg in his windows system and this is very safe.
At your own time, please feel free to jump back in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I hope to be able to start looking at this in February again. I think I lost the VM on which I had the whole setup, and need to set it up to ensure that everything works correctly. For now, I think #51 and #61 can be closed with this.

I'm happy to collaborate, at least in reviewing the issues and doing triage, and helping to close the irrelevant ones.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you @mohits .
I will close #61 and if needed you can open a new one, when you get to it.
Take your time!

@heliocola heliocola merged commit 3085782 into jgarber:master Jan 19, 2024
@mohits
Copy link
Contributor Author

mohits commented Jan 22, 2024

Thank you!

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