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

Fix gem build -C #3954

Closed
wants to merge 2 commits into from
Closed

Fix gem build -C #3954

wants to merge 2 commits into from

Conversation

voxik
Copy link
Contributor

@voxik voxik commented Sep 17, 2020

The directory specified by -C option must be reflected, while the already loaded spec file must be used, wherever it came from. This also fixes confusing messages when .gemspec file was not found at all.

Fixes #3953

I have not tried to run the test suite, so let me see what is going to fail ...

@voxik
Copy link
Contributor Author

voxik commented Sep 18, 2020

I have pushed update which should fix the failing TestGemCommandsBuildCommand#test_can_find_gemspecs_without_dot_gemspec test case (which was IMHO broken from the beginning #1454).

@deivid-rodriguez
Copy link
Member

Thanks @voxik! This looks good to me. Only requests:

  • Could you add a test to cover 15e431b?
  • Could you split the second bug fix to a separate PR?

@deivid-rodriguez
Copy link
Member

Hi @voxik.

I'm now re-reading this patch and I think this does not solve the problem as expected.

The help for the -C flag says

Run as if gem build was started in instead of the current working directory.

However, in this patch, the gemspec is resolved before switching directories, so if it exists in the current directory, it will be taken from there. I would expect the given gemspec to be picked up as is if an absolute path is given, but otherwise be resolved from the path specified in the -C CLI flag.

#3983 has been proposed fixing this issue as I would have expected, but maybe you had other expectations for the -C flag, so if that's the case, let's discuss it.

@voxik
Copy link
Contributor Author

voxik commented Oct 2, 2020

* Could you split the second bug fix to a separate PR?

Done: #3988

The directory specified by `-C` option must be reflected, while the
already loaded spec file must be used, wherever it came from.

Fixes rubygems#3953
@voxik
Copy link
Contributor Author

voxik commented Oct 2, 2020

* Could you add a test to cover [15e431b](https://github.com/rubygems/rubygems/commit/15e431b535d6ae550d1c2d865c06a5e0c90a50f1)?

Done

@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Oct 20, 2020

Just to avoid having two separate PRs fixing the same thing, I'll close this in favor of #3983. If you prefer to change this PR yourself to follow the approach discussed in #3983, I can also close that one and follow up in here, I don't mind either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The gem build -C does not work as expected
3 participants