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

Simplify mkmf-rice, part 2 #212

Merged
merged 1 commit into from
Oct 28, 2024
Merged

Simplify mkmf-rice, part 2 #212

merged 1 commit into from
Oct 28, 2024

Conversation

ankane
Copy link
Contributor

@ankane ankane commented Oct 28, 2024

A few more changes:

  1. Updates IS_MSWIN and IS_MINGW detection to match Ruby as suggested in Simplify mkmf-rice #211
  2. Fixes ld: warning: ignoring duplicate libraries: '-lc++' on Mac (and adds macos-13 to CI for this and future changes)
  3. Removes the cpp_command override (the code hasn't been updated since the bug was filed, but will see if it compiles on CI)

@ankane
Copy link
Contributor Author

ankane commented Oct 28, 2024

Looks like removing the cpp_command override works with Ruby 3.1+ on CI. It also works (edit: does not work) locally with Ruby 3.0 on Mac arm64. I've backed out that change for now.

@ankane ankane mentioned this pull request Oct 28, 2024
@@ -1,7 +1,7 @@
require 'mkmf'

IS_MSWIN = RbConfig::CONFIG['host_os'].match?(/mswin/)
IS_MINGW = RbConfig::CONFIG['host_os'].match?(/mingw/)
IS_MSWIN = /mswin/ =~ RUBY_PLATFORM
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I meant actually using the $constants defined my mkmf already. Unless their and not accessible from the extconf.rb file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, seems like they should be accessible. Feel free to update in a follow-up commit.

@cfis cfis merged commit 8fbf16b into ruby-rice:master Oct 28, 2024
13 checks passed
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.

2 participants