-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Don't install non-default gems to stdlib #4083
Conversation
lib/pom.rb
Outdated
@@ -73,8 +73,8 @@ def initialize( name, version, default_spec = true ) | |||
# :id => 'gem-staging' ) | |||
|
|||
plugin( :clean, | |||
:filesets => [ { :directory => '${basedir}/ruby/gems/shared/specifications/default', | |||
:includes => [ '*' ] }, | |||
:filesets => [ { :directory => '${basedir}/ruby/gems', |
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.
Is it safe to drop the whole dir?
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.
it is safe, but people expect their gems to stayed installed even after a clean build. at least some people do so :)
so I would keep it as it currently is
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.
It couldn't be left as it is, because bundled gems will be installed to ruby/gems/shared/gems
and theirs specifications will be installed to ruby/gems/shared/specifications
.
Second problem with this approach is that all bundled gems need to be added here and this is easy to forget to update this list.
And I didn't understand how to handle SNAPSHOT
versions in this list, because ${jruby-readline.version}
is equal to 1.1.dev-SNAPSHOT
, but gem path should be jruby-readline-1.1.dev-java
.
Also please see #4084 so more state makes build process more unpredictable.
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.
if we move this extra clean into 'dist' profile, then a mvn clean install -Pdist
is doing the right thing.
there is no need to add new gems in this line https://github.com/jruby/jruby/pull/4083/files#diff-a5692734fb1e233c63e88629e6f7bd12L290 - the bin files can be probably omitted here.
please see also my comments below
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.
If I don't add gems dir will look like:
/tmp/0 ⚡ ls jruby-9.1.3.0-SNAPSHOT/lib/ruby/gems/shared/gems/**
jruby-9.1.3.0-SNAPSHOT/lib/ruby/gems/shared/gems/rake-10.4.2:
bin/
jruby-9.1.3.0-SNAPSHOT/lib/ruby/gems/shared/gems/rdoc-4.2.0:
bin/
20ad5e6
to
e1f50dc
Compare
.gitignore
Outdated
lib/ruby/stdlib/hoe* | ||
lib/ruby/stdlib/minitest* | ||
lib/ruby/stdlib/power_assert* |
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.
don't we want to ignore all those whether they are default gems or auxiliary gems - they both will get installed during the build ?
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.
bundled gems will be installed to lib/ruby/gems/shared/gems/
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.
ok.
Where do we stand on this? @mkristian does this seem ok? I don't think this is a release-specific thing, so I'm removing the 9.1.3.0 tag. |
Er, I'm adding the non-release milestone. It was not set to a milestone before. |
c02fc95
to
8f4e5bb
Compare
@headius if we want from my side this PR is ok but it does change the |
8f4e5bb
to
5e40e22
Compare
@mkristian updated and left clean phase intact. |
+1 here ... on the related note - would love a |
still need to look at travis as there are a few failing tests. @kares is |
@mkristian no as I want to be able to undo the stdlib installed files but its a specific requirement thus no blocker here. just mentioning @etehtsea's original (reverted) clean logic might be useful (in corner cases). |
@kares I understood that this is an extra feature. but deleting installed gems is probably not the solution, as the installed files in lib/ruby/stdlib remains. the problem here is that after switching the branch the pom.rb has no more info about the files installed there. such a feature needs to go via |
this has diverged quite esp. now with default and pre-installed gems in 9.2 ... so this would need a rework. |
See #4019 (comment)