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.

Sign up for GitHub

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

Jump to bottom

Implement rich RPM dependencies #124

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Implement rich RPM dependencies #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3bc787
Simplify test from regex to simple includes assertion
ekohl Nov 24, 2023
5d40122
Implement rich RPM dependencies
ekohl Nov 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/gem2rpm/rpm_dependency.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,11 @@ def to_rpm
end
rpm_dependencies.join("\n")
end

# Returns string with entry suitable for RPM .spec file with RPM 4.14+.
def to_rich_rpm
Copy link
Member

Choose a reason for hiding this comment

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

In principle, I don't have anything against this. However, what is the merit of separate method in comparison to adding some options to to_rpm? Maybe the options could also cover my proposal with dropping the version altogether (but now I remember that having the versions around is nice for awareness, so I'd like to keep them printed into the output).

Copy link
Member

Choose a reason for hiding this comment

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

Actually, second option could be to have filter, similar to how the virtualize.with_requires.comment_out filter chain works. Actually that would be probably the best option if it is doable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was under the impression that virtualize.with_requires results in ['Requires: rubygem(mygem) >= 1.0', 'Requires: rubygem(mygem) < 2'] and if you join that, it doesn't work. I considered something like virtualize.to_rich_rpm.with_requires.commented_out but then .to_rich_rpm doesn't return a string anymore.

Actually, second option could be to have filter, similar to how the virtualize.with_requires.comment_out filter chain works. Actually that would be probably the best option if it is doable

I'm not sure what you exactly mean by this. Do you mean supporting a block?

Copy link
Member

Choose a reason for hiding this comment

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

This demonstrates how the filter chain works:

$ cat test.spec.erb 
<%
p development_dependencies
p development_dependencies.virtualize
p development_dependencies.virtualize.with_requires
p development_dependencies.virtualize.with_requires.comment_out
p development_dependencies.virtualize.with_requires.comment_out.to_rpm
-%>

$ bin/gem2rpm --template test.spec.erb test/artifacts/testing_gem/testing_gem-1.0.0.gem 
#<Gem2Rpm::RpmDependencyList:0x00007f10ee78b738 @items=[<Gem::Dependency type=:development name="test_development" requirements="~> 1.0, >= 1.0.0">]>
#<Gem2Rpm::RpmDependencyList:0x00007f10e0fa8dc0 @items=[<Gem::Dependency type=:development name="rubygem(test_development)" requirements="~> 1.0, >= 1.0.0">]>
#<Gem2Rpm::RpmDependencyList:0x00007f10e0fa88c0 @items=[<Gem::Dependency type=:development name="BuildRequires: rubygem(test_development)" requirements="~> 1.0, >= 1.0.0">]>
#<Gem2Rpm::RpmDependencyList:0x00007f10e0fa82f8 @items=[<Gem::Dependency type=:development name="# BuildRequires: rubygem(test_development)" requirements="~> 1.0, >= 1.0.0">]>
"# BuildRequires: rubygem(test_development) >= 1.0\n# BuildRequires: rubygem(test_development) < 2\n# BuildRequires: rubygem(test_development) >= 1.0.0\n"

As you can see, there still single Gem::Dependency object and the filters modifies the internal name field, keeping the requirement field in its original form. IOW it is still array of Gem::Version object.

Let me change the template a bit:

$ cat test.spec.erb 
<%
development_dependencies.virtualize.with_requires.comment_out.each {|d| p d.requirement }
-%>

$ bin/gem2rpm --template test.spec.erb test/artifacts/testing_gem/testing_gem-1.0.0.gem 
[">= 1.0", "< 2", ">= 1.0.0"]

IOW, at this stage, it is still possible to output single line or three lines, as needed. From here I continued in this direction:

$ cat test.spec.erb 
<%=
development_dependencies.virtualize.with_requires.comment_out.map do |d|
  [d.name, "(#{d.requirement.join(' with ')})"].join
end.join("\n")
-%>

$ bin/gem2rpm --template test.spec.erb test/artifacts/testing_gem/testing_gem-1.0.0.gem 
# BuildRequires: rubygem(test_development)(>= 1.0 with < 2 with >= 1.0.0)

This however unveils, that my filter idea won't work as I hoped 🤣🤦🏻‍♂️ But after all, this might be the way:

$ cat test.spec.erb 
<%=
dd = development_dependencies.virtualize.map do |d|
  Gem::Dependency.new("(#{d.requirement.map {|r| [d.name, r].join(' ')}.join(' with ')})", d.type)
end
dd = Gem2Rpm::RpmDependencyList.new dd
dd.with_requires.comment_out.to_rpm
-%>

$ bin/gem2rpm --template test.spec.erb test/artifacts/testing_gem/testing_gem-1.0.0.gem 
# BuildRequires: (rubygem(test_development) >= 1.0 with rubygem(test_development) < 2 with rubygem(test_development) >= 1.0.0)

This could be further improved if the RpmDependencyList implemented the map method or of course if there was different filter, such as .rich_dependencies hiding this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This however unveils, that my filter idea won't work as I hoped

This is exactly what I was referring to.

I'll say that I actually like the loop in the ERB template: it makes it easier to understand what's going on. Implementing map (or even more parts of Enumerable) on RpmDependencyList does sound like a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

Implementing the map or additional filter provides just an option. It does not limit use of more low level constructs. After all, we certainly had more bare loops in the templates previously.

rpm_dependencies = requirement.map { |v| v.to_s.empty? ? name : "#{name} #{v}" }
rpm_dependencies.size == 1 ? rpm_dependencies.first : "(#{rpm_dependencies.join(' with ')})"
end
end
end
6 changes: 3 additions & 3 deletions templates/fedora-27-rawhide.spec.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ BuildRequires: <%= requirement "ruby#{'-devel' unless spec.extensions.empty?}",
# https://fedoraproject.org/wiki/Packaging:C_and_C++#BuildRequires_and_Requires
BuildRequires: gcc
<% end -%>
<%= development_dependencies.reject do |d|
["rdoc", "rake", "bundler"].include? d.name
end.virtualize.with_requires.comment_out.to_rpm -%>
<% for req in development_dependencies.reject { |d| ["rdoc", "rake", "bundler"].include? d.name }.virtualize -%>
# BuildRequires: <%= req.to_rich_rpm %>
Copy link
Member

Choose a reason for hiding this comment

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

Unless I am missing something, I'd prefer to keep using the end.virtualize.with_requires.comment_out and just append the to_rich_rpm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant to reply https://github.com/fedora-ruby/gem2rpm/pull/124/files#r1406987179 here.

<% end -%>
<% if spec.extensions.empty? -%>
BuildArch: noarch
<% end -%>
Expand Down
2 changes: 1 addition & 1 deletion test/templates/test_fedora.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def test_exclude_extension_directory
end

def test_build_requires
assert_match(/^# BuildRequires: rubygem\(test_development\) >= 1\.0\.0$/, @out_string)
assert_includes(@out_string, "\n# BuildRequires: (rubygem(test_development) >= 1.0 with rubygem(test_development) < 2 with rubygem(test_development) >= 1.0.0)\n")
Copy link
Member

Choose a reason for hiding this comment

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

Just to double check, is RPM able to cope with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://fedoraproject.org/wiki/Changes/RPM-4.14#User_Experience lists Requires: (foo >= 1.0 with foo < 2.0) as an example. I haven't tried more 2 conditions, so that would be a good testcase. That said, there is this code in /usr/lib/rpm/rubygems.req (rubygems-devel-3.4.10-180.fc38.noarch)

# Compose dependency together with its requirements in RPM rich dependency
# string.
def self.compose_dependency_string(name, requirements)
  dependency_strings = requirements.map { |requirement| name + requirement }
  dependency_string = dependency_strings.join(' with ')
  dependency_string.prepend('(').concat(')') if dependency_strings.length > 1
  dependency_string
end

That doesn't make any special conditions and does pretty much the same thing.

Copy link
Member

Choose a reason for hiding this comment

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

I can't tell I trust myself :D

end

def test_rubygems_is_not_required
Expand Down
Loading