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

avoid false const warnings (when using bundler) #390

Closed
wants to merge 2 commits into from

Conversation

kares
Copy link
Contributor

@kares kares commented Feb 28, 2019

Bundler will load YAML and thus Psych, which causes versions.rb to be
required, but than it also loads the psych.gemspec ... (loading .rb twice)

in such case, the reported version constants might not be accurate
and also causes warnings due double loading of the same versions.rb

this is really annoying and the unless defined?(VERSION) is not really a good solution
(have been confused by the psych version used - depends on LOAD_PATH entries order)

Bundler will load YAML and thus Psych, which causes versions.rb to be
required, but it also loads the psych.gemspec ...

in such case, the reported version constants might not be accurate
and also causes warnings due double loading of the same versions.rb
(see previous commit for explanation)
@stomar
Copy link
Contributor

stomar commented Mar 3, 2019

I did not investigate into the problem this is supposed to solve (possibly a problem of bundler?), but parsing a Ruby file using regular expressions instead of simply require‘ing it seems really wrong. Also, the proposed change does not consider the special treatment necessary for the core repository.

@deivid-rodriguez
Copy link
Contributor

This is interesting. We have this longstanding PR in bundler that seems to be somehow related. How can this problem be reproduced?

@kares
Copy link
Contributor Author

kares commented Feb 11, 2020

any objections to merge this one?

@deivid-rodriguez
Copy link
Contributor

I tried a better fix on #433 but I'm still unsure how to reliably reproduce these warnings... 🤔

@kares
Copy link
Contributor Author

kares commented Feb 14, 2020

vaguely recall these came while doing day-to-day dev on JRuby and than installing a custom psych.
believe might have been trying out a dev built before releasing but kept on running into these with some of the test suite.

#433 might do - the gist of the changes were to not load any files from the gem while reading the spec and #433 keeps loading version.rb which than would be the source of confusion about the version used (if it gets loaded again as a bundled dependency). agree, should have had a reproducer.

@deivid-rodriguez
Copy link
Contributor

deivid-rodriguez commented Feb 14, 2020

Ok, I can reliably reproduce it now. Do gem install psych, so that you have a global version of psych installed, and then:

On master

$ rm Gemfile.lock && bundle
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.headius.backport9.modules.Modules to method sun.nio.ch.NativeThread.signal(long)
WARNING: Please consider reporting this to the maintainers of com.headius.backport9.modules.Modules
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
/home/deivid/.rbenv/versions/jruby-9.2.9.0/lib/ruby/stdlib/psych/versions.rb:8: warning: already initialized constant DEFAULT_SNAKEYAML_VERSION
Fetching gem metadata from https://rubygems.org/..................
Resolving dependencies...
Using rake 13.0.1
Using bundler 2.1.4
Using jar-dependencies 0.4.0
Using minitest 5.14.0
Using psych 3.1.0 (java) from source at `.`
  jar dependencies for psych-3.1.0-java.gemspec . . .
[DEPRECATED] `Bundler.with_clean_env` has been deprecated in favor of `Bundler.with_unbundled_env`. If you instead want the environment before bundler was originally loaded, use `Bundler.with_original_env` (called at /home/deivid/.rbenv/versions/jruby-9.2.9.0/lib/ruby/gems/shared/gems/ruby-maven-3.3.12/lib/ruby_maven.rb:38)
unsupported Java version "11", defaulting to 1.7
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.jruby.util.io.FilenoUtil (file:/home/deivid/.m2/repository/org/jruby/jruby-core/9.1.2.0/jruby-core-9.1.2.0.jar) to method sun.nio.ch.SelChImpl.getFD()
WARNING: Please consider reporting this to the maintainers of org.jruby.util.io.FilenoUtil
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
sh: 1: cd: can't cd to uri:classloader:/
      org.yaml:snakeyaml:1.23:compile
Using rake-compiler 1.1.0
Using ruby-maven-libs 3.3.9
Using ruby-maven 3.3.12
Bundle complete! 4 Gemfile dependencies, 8 gems now installed.
Use `bundle info [gemname]` to see where a bundled gem is installed.

On my branch

$ rm Gemfile.lock && bundle
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.headius.backport9.modules.Modules to method sun.nio.ch.NativeThread.signal(long)
WARNING: Please consider reporting this to the maintainers of com.headius.backport9.modules.Modules
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
Fetching gem metadata from https://rubygems.org/..................
Resolving dependencies...
Using rake 13.0.1
Using bundler 2.1.4
Using jar-dependencies 0.4.0
Using minitest 5.14.0
Using psych 3.1.0 (java) from source at `.`
  jar dependencies for psych-3.1.0-java.gemspec . . .
[DEPRECATED] `Bundler.with_clean_env` has been deprecated in favor of `Bundler.with_unbundled_env`. If you instead want the environment before bundler was originally loaded, use `Bundler.with_original_env` (called at /home/deivid/.rbenv/versions/jruby-9.2.9.0/lib/ruby/gems/shared/gems/ruby-maven-3.3.12/lib/ruby_maven.rb:38)
unsupported Java version "11", defaulting to 1.7
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.jruby.util.io.FilenoUtil (file:/home/deivid/.m2/repository/org/jruby/jruby-core/9.1.2.0/jruby-core-9.1.2.0.jar) to method sun.nio.ch.SelChImpl.getFD()
WARNING: Please consider reporting this to the maintainers of org.jruby.util.io.FilenoUtil
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
sh: 1: cd: can't cd to uri:classloader:/
      org.yaml:snakeyaml:1.23:compile
Using rake-compiler 1.1.0
Using ruby-maven-libs 3.3.9
Using ruby-maven 3.3.12
Bundle complete! 4 Gemfile dependencies, 8 gems now installed.
Use `bundle info [gemname]` to see where a bundled gem is installed.

The problem is that when loading the gemspec, bundler loads the versions.rb file relatively, but then bundler internally tries to load psych from the $LOAD_PATH and loads the globally installed version, causing the redefinition warnings.

I think it's fine to load the version file from the gemspec as #433 do. Most gems out there do it and it works fine. Note that it's only for "development", once a gem is pushed to rubygems.org and installed, the gemspec has the correct version and does not load any external files.

The only thing special about psych that is causing this warning is that it is loaded by bundler internally on bundle install. In this case, I think it's fine to modify the $LOAD_PATH for development of the psych gem inside the gemspec to force bundler to actually load the local version?

@kares
Copy link
Contributor Author

kares commented Feb 14, 2020

Thanks for looking into it David, closing in favor of #433

@kares kares closed this Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants