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 exclusion of .gemspec file itself in bundle gem generated gemspec file #7488

Merged
merged 1 commit into from Mar 5, 2024

Conversation

nobu
Copy link
Contributor

@nobu nobu commented Feb 25, 2024

What was the end-user or developer problem that led to this PR?

When building by gem, __FILE__ is the path name given in the command line, or the gemspec file name in the current directory. In that case, comparison it and expanded path never equal.

What is your fix for the problem, implemented in this PR?

List file names by using chdir option of IO.popen, and compare them with the base name of __FILE__ instead.

Also redirect stderr git ls-files to null without shelling out.

Make sure the following tasks are checked

- Redirect stderr `git ls-files` to null without shelling out.

- When building by `gem`, `__FILE__` is the path name given in the
  command line, or the gemspec file name in the current directory.  In
  that case, comparison it and expanded path never equal.  Compare
  listed file names with the base name of `__FILE__` instead.
Comment on lines -30 to +33
spec.files = Dir.chdir(__dir__) do
`git ls-files -z`.split("\x0").reject do |f|
(File.expand_path(f) == __FILE__) ||
gemspec = File.basename(__FILE__)
spec.files = IO.popen(%w[git ls-files -z], chdir: __dir__, err: IO::NULL) do |ls|
ls.readlines("\x0", chomp: true).reject do |f|
(f == gemspec) ||
Copy link
Member

Choose a reason for hiding this comment

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

This was already a complicated line to drop into a new gem. The change makes it even more complicated.

Should we add a helper to spec that does this? Maybe:

spec.files = spec.git_files_excluding(__FILE__, "bin", "spec")

I know this is a more involved change. We could make a temporary fix while we wait for improved syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe a class method?
It feels questionable to me that Gem::Specification instances should have such method.

Copy link
Contributor Author

@nobu nobu Feb 28, 2024

Choose a reason for hiding this comment

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

BTW, what version of git is required in the current rubygems?
Since git 1.9, exclude "magic signature" pathspecs are available.

excludes = %w[bin/ test/ spec/ features/ .git <%= config[:ci_config_path] %>appveyor Gemfile]

dir, gemspec = File.split(__FILE__)
excludes = [dir, *excludes].map {|e| ":^#{e}"}
spec.files = (IO.popen(*%w[git ls-files -z --], *excludes, chdir: dir) do |f|
                f.readlines("\0", chomp: true)
              end rescue [])

Copy link
Member

Choose a reason for hiding this comment

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

BTW, what version of git is required in the current rubygems?

I think we must support the version of git that ships with OSX. We've never made a real requirement that I know of.

@hsbt, what do you think? I don't feel strongly myself and I would be fine merging this if others are ok with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On macOS 14.3.1:

$ /usr/bin/git --version
git version 2.39.3 (Apple Git-145)

Not sure about older versions.

Copy link
Member

Choose a reason for hiding this comment

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

I investigate git versions of debian and ubuntu.

  • Ubuntu 22.04: 2.34.1
  • Debian 12: 2.39.2

We should support git-2.34.x at least.

Copy link
Contributor Author

@nobu nobu Mar 24, 2024

Choose a reason for hiding this comment

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

@hsbt Thanks, git 2.x is enough for the "magic signatures".

I found this description in Xcode 12 Release Notes.

New Features
You can now specify the default branch name for new repositories in the Source Control pane of Xcode’s Preferences. This preference uses the init.defaultBranch Git config option available in Git version 2.28. (66232985)

Copy link
Member

Choose a reason for hiding this comment

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

We don't have an official "lowest supported version" but we do ship code to make sure to play nice with git older than 1.8.5.

If I recall correctly, the motivation was that CentOS 7 shipped with a very old git version. CentOS 7 reaches its end of life by the 30th of June, I think it'd be ok to drop support after that an officially declare an official lowest supported version like 2.x.

@hsbt hsbt merged commit 1ab7879 into rubygems:master Mar 5, 2024
83 checks passed
@deivid-rodriguez deivid-rodriguez changed the title Use IO.popen to list files Fix exclusion of .gemspec file itself in bundle gem generated gemspec file Mar 18, 2024
deivid-rodriguez pushed a commit that referenced this pull request Mar 20, 2024
Use `IO.popen` to list files

(cherry picked from commit 1ab7879)
@nobu nobu deleted the popen-in-gemspec branch March 24, 2024 12:58
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.

None yet

5 participants