Improve gem build -C flag #3983
Conversation
There was a problem hiding this comment.
Nice @bronzdoc, this looks very good. I made some changes locally:
-
gem builddoes support building multiple gemspecs in a single invokation. However, in this patch, we're taking the first matching gemspec and ignoring the rest, so the user might be left wondering why the glob argument was happily accepted but only one gem got built. So I reusedfind_gemspec, which raises if multiple matching gemspecs are found. -
I noticed that the two errors about the gemspec not being found were essentially the same thing and could be made more clear. So I merged all that logic into a single message that a gemspec could not be inferred from the argument given to
gem build, or that it couldn't find any gemspecs at all if no argument was given.
This is the patch:
diff --git a/lib/rubygems/commands/build_command.rb b/lib/rubygems/commands/build_command.rb
index d8aeb49223..b70d21fe60 100644
--- a/lib/rubygems/commands/build_command.rb
+++ b/lib/rubygems/commands/build_command.rb
@@ -63,20 +63,18 @@ def usage # :nodoc:
def execute
if build_path = options[:build_path]
Dir.chdir(build_path) do
- gem_name = get_one_optional_argument || find_gemspec
- build_gem(gem_name)
+ build_gem
end
return
end
- gem_name = get_one_optional_argument || find_gemspec
- build_gem(gem_name)
+ build_gem
end
private
- def find_gemspec
- gemspecs = Dir.glob("*.gemspec").sort
+ def find_gemspec(glob = "*.gemspec")
+ gemspecs = Dir.glob(glob).sort
if gemspecs.size > 1
alert_error "Multiple gemspecs found: #{gemspecs}, please specify one"
@@ -86,22 +84,19 @@ def find_gemspec
gemspecs.first
end
- def build_gem(gem_name)
- gemspec = Dir.glob(gem_name).sort.first || "#{gem_name}.gemspec"
+ def build_gem
+ gemspec = resolve_gem_name
- if File.exist?(gemspec)
- spec = Gem::Specification.load(gemspec)
- build_package(spec)
- elsif gemspec.scan(".gemspec").size > 1
- alert_error "No Gemspec in #{Dir.pwd}"
- terminate_interaction(1)
+ if gemspec
+ build_package(gemspec)
else
- alert_error "Gemspec file not found: #{gemspec}"
+ alert_error error_message
terminate_interaction(1)
end
end
- def build_package(spec)
+ def build_package(gemspec)
+ spec = Gem::Specification.load(gemspec)
if spec
Gem::Package.build(
spec,
@@ -114,4 +109,26 @@ def build_package(spec)
terminate_interaction 1
end
end
+
+ def resolve_gem_name
+ return find_gemspec unless gem_name
+
+ if File.exist?(gem_name)
+ gem_name
+ else
+ find_gemspec("#{gem_name}.gemspec") || find_gemspec(gem_name)
+ end
+ end
+
+ def error_message
+ if gem_name
+ "Couldn't find a gemspec file matching '#{gem_name}' in #{Dir.pwd}"
+ else
+ "Couldn't find a gemspec file in #{Dir.pwd}"
+ end
+ end
+
+ def gem_name
+ get_one_optional_argument
+ end
end
diff --git a/test/rubygems/test_gem_commands_build_command.rb b/test/rubygems/test_gem_commands_build_command.rb
index 43a312eb16..2acc41abfa 100644
--- a/test/rubygems/test_gem_commands_build_command.rb
+++ b/test/rubygems/test_gem_commands_build_command.rb
@@ -231,7 +231,7 @@ def test_execute_missing_file
end
assert_equal '', @ui.output
- assert_equal "ERROR: Gemspec file not found: some_gem.gemspec\n", @ui.error
+ assert_equal "ERROR: Couldn't find a gemspec file matching 'some_gem' in #{@tempdir}\n", @ui.error
end
def test_execute_outside_dir
@@ -335,7 +335,7 @@ def test_execute_outside_dir_no_gemspec_present
end
assert_equal "", @ui.output
- assert_equal "ERROR: No Gemspec in #{gemspec_dir}\n", @ui.error
+ assert_equal "ERROR: Couldn't find a gemspec file matching '*.gemspec' in #{gemspec_dir}\n", @ui.error
gem_file = File.join gemspec_dir, File.basename(@gem.cache_file)
refute File.exist?(gem_file)However, I feel the way we are interpreting -C might not be the use case @voxik had in mind, so we might want to wait for feedback from him.
|
I think this proposal is/keeps breaking the initial premise which was there from beginning, i.e. the But I agree that |
|
@voxik I think you are correct 👍. We should fix this your way, and change the docs for
Would that make sense? |
|
Actually, not really convinced after a second thought. Thinking of
If we introduce a new flag, shouldn't it be able to define its own behaviour? I mean, the behaviour you propose might be more useful, but I don't see how a flag shouldn't modify the way arguments are interpreted. Specially |
|
TBH, I have never really cared about having This behavior (available since RubyGems inception) was broken by 90e6768 and I complained about it in #2587, where the idea with If the above can be shortened to: that is fine, but please don't make any assumption about where the ".gemspec" file lives. Guessing if the "json.gemspec" - when the |
|
Also, I should mention that this is what current help says: It does not make any assumptions about where the But again, this was broken by #2859 / #2887 and neither #1454 helped here, where the assumption about "file" was relaxed for the first time, but nobody updated the specification. |
|
Yes, I'm aware that we introduced the And you're also right the the Would #3983 (comment) work for you? That's the approach your PR takes, right? |
Yes to both 👍 |
|
Or possibly
|
|
s/parameter/option/ for consistency |
|
Ok. @bronzdoc I think we should take @voxik's suggested path, since it's the traditional expectation for It deviates a bit from standard |
|
I need to catchup with all comments. I will give them a look later tonight |
|
Something else I was thinking is that if we had gone with #2587 (comment), this shouldn't be an issue for you and you wouldn't need |
|
I like this idea the most
|
I'm not sure I understand your idea. Does this mean that you propose to try to find the gemspec relative to the If that is the proposal, I just worry, that the implementation will get even more complex and error prone with no benefit. |
|
No, I'm proposing that we move forward with the approach in this PR. The thing is, I misunderstood the history of these issues. In #3983 (comment) you said the following (talking about
This is correct, but as far as I understand, we considered this as an accidental regression and restored the original behaviour, so The In addition to that, you just said in #3983 (comment) that shortening things to So, it seems to me that this PR is the way to go, with the only extra addition of improving the |
|
The problem is that you tried to take everything I said while trying to provide you examples close to the real life scenarios, and you applies it to the provided patch. So let me share two real life scenarios. How the #2587 startedHere is snippet from Fedora .spec And now how does it look under the hood: This is the patch adding RPM support for .gem packages, where you can see the way This use case is not related to the Now the story of #3953I have received voxik/abrt-ruby#10. The idea behind this PR is to test upstream package in the context of Fedora. Specifically, this is line of the interest: This unexpectedly moved all the *.gem files into the and the rest is in #3953. Actually, re-reading my initial comment, the surprise was not just about where the *.gemspec is located and expected, but also where the resulting *.gem package is created. And frankly, the whole idea of using |
|
The rubygem-abrt was probably too simplistic, because somebody might wonder, why we repack the gems. So please take a look on rubygem-idn instead: https://src.fedoraproject.org/rpms/rubygem-idn/blob/master/f/rubygem-idn.spec I.e. we have to fix the C extension to be able to |
|
Also, I think that there should also be taken in account what is the influence of shell expansion. Please note that there was used |
|
Ok, I understand this better now. Everything regarding passing relative paths in a different folder to I'm happy to review your new use case and give it some thought and see what we can add/change to make things work better for you. But I don't think the solution is changing the specification of Rmember that I was very clear about this when I proposed the specification for the option:
And you intuitively liked the idea:
The fact that you found a new use case where you thought
Shell expansion (if your shell is configured to do it) happens before us, so I'm not sure what we could do about it. |
|
First of all, I am glad for this discussion, because if nothing else, I hope that the behavior will be better specified.
And possibly not considering all implications and consequences 😉 I get your argument with shell expansion. Therefore, while it is a bit out of scope and should have been covered probably elsewhere but, could this PR be extended by two (actually four) test cases? Given that the CWD is the gem directory:
|
|
Sure that seems like a good idea. @bronzdoc Could you add tests for those cases (or unify them if some of the cases is already tested), when you are able to loop back to this PR? |
|
Looking at the test suite, the irony is that almost all test are executed with absolute path to the .gemspec file. I don't have the feeling this represents the real life usage. This could be the two test cases BTW: diff --git a/test/rubygems/test_gem_commands_build_command.rb b/test/rubygems/test_gem_commands_build_command.rb
index 167752eec..3669a3edd 100644
--- a/test/rubygems/test_gem_commands_build_command.rb
+++ b/test/rubygems/test_gem_commands_build_command.rb
@@ -272,6 +272,88 @@ def test_execute_outside_dir
assert_equal "this is a summary", spec.summary
end
+ def test_execute_outside_dir_with_external_gemspec
+ gemspec_dir = File.join @tempdir, 'gemspec_dir'
+ gemspec_file = File.join gemspec_dir, @gem.spec_name
+
+ gemcode_dir = File.join @tempdir, 'build_command_gem'
+ readme_file = File.join gemcode_dir, 'README.md'
+
+ FileUtils.mkdir_p gemspec_dir
+ FileUtils.mkdir_p gemcode_dir
+
+ File.open readme_file, 'w' do |f|
+ f.write "My awesome gem in nested directory"
+ end
+
+ File.open gemspec_file, 'w' do |gs|
+ gs.write @gem.to_ruby
+ end
+
+ @cmd.options[:build_path] = gemcode_dir
+ @cmd.options[:args] = [gemspec_file]
+
+ use_ui @ui do
+ @cmd.execute
+ end
+
+ output = @ui.output.split "\n"
+ assert_equal " Successfully built RubyGem", output.shift
+ assert_equal " Name: some_gem", output.shift
+ assert_equal " Version: 2", output.shift
+ assert_equal " File: some_gem-2.gem", output.shift
+ assert_equal [], output
+
+ gem_file = File.join gemcode_dir, File.basename(@gem.cache_file)
+ assert File.exist?(gem_file)
+
+ spec = Gem::Package.new(gem_file).spec
+
+ assert_equal "some_gem", spec.name
+ assert_equal "this is a summary", spec.summary
+ end
+
+ def test_execute_outside_dir_with_external_relative_gemspec
+ gemspec_dir = File.join @tempdir, 'gemspec_dir'
+ gemspec_file = File.join gemspec_dir, @gem.spec_name
+
+ gemcode_dir = File.join @tempdir, 'build_command_gem'
+ readme_file = File.join gemcode_dir, 'README.md'
+
+ FileUtils.mkdir_p gemspec_dir
+ FileUtils.mkdir_p gemcode_dir
+
+ File.open readme_file, 'w' do |f|
+ f.write "My awesome gem in nested directory"
+ end
+
+ File.open gemspec_file, 'w' do |gs|
+ gs.write @gem.to_ruby
+ end
+
+ @cmd.options[:build_path] = gemcode_dir
+ @cmd.options[:args] = [File.join("..", "gemspec_dir", @gem.spec_name)]
+
+ use_ui @ui do
+ @cmd.execute
+ end
+
+ output = @ui.output.split "\n"
+ assert_equal " Successfully built RubyGem", output.shift
+ assert_equal " Name: some_gem", output.shift
+ assert_equal " Version: 2", output.shift
+ assert_equal " File: some_gem-2.gem", output.shift
+ assert_equal [], output
+
+ gem_file = File.join gemcode_dir, File.basename(@gem.cache_file)
+ assert File.exist?(gem_file)
+
+ spec = Gem::Package.new(gem_file).spec
+
+ assert_equal "some_gem", spec.name
+ assert_equal "this is a summary", spec.summary
+ end
+
def test_execute_outside_dir_with_glob_argument
gemspec_dir = File.join @tempdir, 'build_command_gem'
gemspec_file = File.join gemspec_dir, @gem.spec_nameThe first is taken from my #3954. The |
|
Great! I think it makes sense to include your test in this PR and close it in favour of this one? |
bronzdoc
force-pushed
the
improve-c-flag
branch
from
283bfc7
to
a46c11b
Compare
There was a problem hiding this comment.
This looks good to me!
@voxik What do you think?
By the way, I think we should move the -C flag to a global option since I believe it would be useful outside of a gem build context. See #448 (comment). If we agree on it, I can open a separate ticket to remember about it.
|
I'll go ahead and merge this. We can always iterate for further improvements. |
Improve gem build -C flag (cherry picked from commit 2ad192e)
Improve gem build -C flag (cherry picked from commit 2ad192e)
Description:
Fixes #3953.
Closes #3954.
Consolidate the current behavior of "gem build" with
gem build -C [PATH]I will abide by the code of conduct.