#19 Add macros to edit files in .gemspec
Merged a year ago by pvalena. Opened a year ago by pvalena.
rpms/ pvalena/ruby gemspec-files  into  master

file modified
+67 -2

@@ -42,8 +42,8 @@ 

  }

  

  

- # The 'read' command in gemspec_add_dep and gemspec_remove_dep macros is not

- # essential, but it is usefull to make the script appear in build log.

+ # The 'read' command in %%gemspec_* macros is not essential, but it is usefull

+ # to make the sript appear in build log.

  

  

  # %gemspec_add_dep - Add dependency into .gemspec.

@@ -128,3 +128,68 @@ 

  echo "$gemspec_remove_dep_script" | ruby \

  unset -v gemspec_remove_dep_script \

  %{nil}

+ 

+ 

+ # %%gemspec_add_file - Add files to various files lists in .gemspec.

+ #

+ # Usage: %%gemspec_add_file [options] <file>

+ #

+ # Add files to .gemspec file. <file> is expected to be valid Ruby code.

+ # Path to file is expected. Does not check real files in any way.

+ # By default, `files` list is edited.

+ #

+ # -s <gemspec_file>   Overrides the default .gemspec location.

+ # -t                  Edit test_files only.

+ # -r                  Edit extra_rdoc_files only.

+ #

+ %gemspec_add_file(s:tr) \

+ read -d '' gemspec_add_file_script << 'EOR' || : \

+   gemspec_file = '%{-s*}%{!?-s:%{_builddir}/%{gem_name}-%{version}.gemspec}' \

+   \

+   abort("gemspec_add_file: Use only one '-t' or '-r' at a time.") if "%{?-t}%{?-r}" == "-t-r" \

+   \

+   filenames = %{*}%{!?1:nil} \

+   filenames = Array(filenames) \

+   \

+   spec = Gem::Specification.load(gemspec_file) \

+   abort("#{gemspec_file} is not accessible.") unless spec \

+   \

+   spec.%{?-t:test_}%{?-r:extra_rdoc_}files += filenames \

+   File.write gemspec_file, spec.to_ruby \

+ EOR\

+ echo "$gemspec_add_file_script" | ruby \

+ unset -v gemspec_add_file_script \

+ %{nil}

+ 

+ 

+ # %%gemspec_remove_file - Remove files from various files lists in .gemspec.

+ #

+ # Usage: %%gemspec_remove_file [options] <file>

+ #

+ # Remove files from .gemspec file. <file> is expected to be valid Ruby code.

+ # Path to file is expected. Does not check/remove real files in any way.

+ # By default, `files` list is edited. File has to be removed from `test_files`

+ # first in order to be removable from `files`.

+ #

+ # -s <gemspec_file>   Overrides the default .gemspec location.

+ # -t                  Edit test_files only.

+ # -r                  Edit extra_rdoc_files only.

+ #

+ %gemspec_remove_file(s:tr) \

+ read -d '' gemspec_remove_file_script << 'EOR' || : \

+   gemspec_file = '%{-s*}%{!?-s:%{_builddir}/%{gem_name}-%{version}.gemspec}' \

+   \

+   abort("gemspec_remove_file: Use only one '-t' or '-r' at a time.") if "%{?-t}%{?-r}" == "-t-r" \

+   \

+   filenames = %{*}%{!?1:nil} \

+   filenames = Array(filenames) \

+   \

+   spec = Gem::Specification.load(gemspec_file) \

+   abort("#{gemspec_file} is not accessible.") unless spec \

+   \

+   spec.%{?-t:test_}%{?-r:extra_rdoc_}files -= filenames \

+   File.write gemspec_file, spec.to_ruby \

+ EOR\

+ echo "$gemspec_remove_file_script" | ruby \

+ unset -v gemspec_remove_file_script \

+ %{nil}

file modified
+4

@@ -1068,6 +1068,10 @@ 

  %{gem_dir}/specifications/xmlrpc-%{xmlrpc_version}.gemspec

  

  %changelog

+ * Thu May 10 2018 Pavel Valena <pvalena@redhat.com> - 2.5.1-93

+ - Add macros to edit files lists in .gemspec

+   (gemspec_add_file and gemspec_remove_file).

+ 

  * Wed May 02 2018 Vít Ondruch <vondruch@redhat.com> - 2.5.1-93

  - Make %%gemspec_{add,remove}_dep modify .gemspec provided by %%setup macro.

  

Specifically, gemspec_add_file, to add; and gemspec_remove_file remove files.
Also set default gemspec path to ../%{gem_name}-%{version}.gemspec.

Up-to-date scratch-build:
https://koji.fedoraproject.org/koji/taskinfo?taskID=26992527

You don't want to commit this with the new gemspec_{add,remove}_file macros, do you? I actually have commit with similar changes ready locally and I am about to push it.

Is this the right approach? Check the "requirements" in the gemspec_remove_dep macro. This allows greater flexibility in what is submitted there. You can submit for example array, or it can be some object, but you restrict the filename to be just string. I think this deserves to be more generic.

I guess that "gemspec_{add,remove}_" or even "gempsec_" would be enough. This starts to be too wordy.

No, you're right, I dont. I did expect you to pick the changes, or me later to split this into separate commit.

I did consider passing Ruby code, but decided that simple regex (f =~ /#{filename}/) is as much flexible as one needs. And I can't find a counter-example, apart from actually passing list of files, where I'd rather use this macro in a loop.

We could detect whether regex or enumerable (ducktyping-wise) was passed and behave accordingly. IMHO it would bloat the code a bit, so I opted for most simplistic solution.

Alternatively, we could simply yield the files. WDYT?

Here it definitely makes sense to pass a Ruby code.

So how about starting with something like:

filenames = %{*}%{!?1:nil}
filenames = Array(filenames)

and later just:

spec.files -= filenames

TBH, the RegExp is nice, but it is big hammer IMO. But you should still be able to use Ruby to get the files list, e.g. if you use the macro like this (untested):

%gemspec_remove_file Dir.glob('*')
%gemspec_remove_file Dir.glob('*').reject {|f| f =~ /foo/ }

filenames = Array(filenames)

At that point, you could have anything, and you do expect it to be initializer for Array. I'm not quite sure that's more generic. I'd rather have

filenames = [%{*}%{!?1:nil}].flatten
files do |f|
  filenames.reject { |n| f === n  }
end

Which works both for strings and regexps.

%gemspec_remove_file Dir.glob('*').reject {|f| f =~ /foo/ }

The issue with this is that the files do not exist at that point.

Kernel#Array() tries to convert everything into array using to_ary, to_a or simply returning the argument as [arg]. And that is precisely what I want. That is very flexible approach, but anyway, I expect most people will use two basic approaches:

 %gemspec_remove_file "somefile"
 %gemspec_remove_file ["somefile", "otherfile"]

Actually, I don't like the RegExp at all. People should be specific what they want to remove. And the glob will work as long as you expand the gem using %setup macro and remove the file from the system prior you do the "rm".

This actually leads me to question, shouldn't the macro try to remove the file from the filesystem?

Kernel#Array() tries to convert everything into array using to_ary, to_a or simply returning the argument as [arg]. And that is precisely what I want. That is very flexible approach, but anyway, I expect most people will use two basic approaches:
%gemspec_remove_file "somefile"
%gemspec_remove_file ["somefile", "otherfile"]

I was simply trying to list a directory, but that can be done like you said in case it does exist.

Actually, I don't like the RegExp at all. People should be specific what they want to remove. And the glob will work as long as you expand the gem using %setup macro and remove the file from the system prior you do the "rm".

I see.

This actually leads me to question, shouldn't the macro try to remove the file from the filesystem?

Now, that would make sense; and could actually solve the issue with the order of commands I have.

Let me prepare some other implementation.

rebased onto d69aa13c48f46f9dac89778b8dd223e0a808b035

a year ago

rebased onto adb6c4f8d82aa92d6ee47627538823c647b8a868

a year ago

There is one missing thing. There are not just "files", but also "test_files". It would be probably useful to cover "test_files" as well. But that could be separate PR.

Just FTR, I am testing with rubygem-fog.

Also please add ruby.spec changelog entry (while keep the same release).

This actually leads me to question, shouldn't the macro try to remove the file from the filesystem?

Now, that would make sense; and could actually solve the issue with the order of commands I have.

Trying the proposed macros and the "-r" flag, I am not sure anymore about usefulness of this option. If the files are not listed in the .gemspec, they won't be included in the rebuild gem and therefore there is no extra need to remove them from the filesystem. For example if one wants to remove the "vendor" directory such as:

%gemspec_remove_file -r Dir.glob('vendor/*')

I'd still rather see some explicit "rm -rf vendor" around to be really sure that the directory won't appear in the rebuilt gem. Also, the macro removes just files, not directories and removing directories is useless for .gemspec file while it would add additional complexity to the macro.

rebased onto 7f98f5d0b8502b4d44bf5fed93577b61da59ada7

a year ago

rebased onto 2f2b910bb5bb0154cf44188ccc80c9f399852cb9

a year ago

I had to add additional spec.files -= filenames \ to remove macro, because when adding to test_files file also popped in files.

The "abort" need some message of course, e.g. "-t and -r options are exclusive."

I had to add additional spec.files -= filenames \ to remove macro, because when adding to test_files file also popped in files.

I don't think this is good idea. It this should be consistent, I see two options:

1) Either we remove specified files from all their possible locations, e.g.:

spec.files -= filenames \
spec.test_files -= filenames \
spec.extra_rdoc_files -= filenames \

and then we don't need any options. Or

2) There should be possible to remove the files selectively from the file lists and then this must be done exclusively on specified file list. Of course that means to call the macro twice, but it still achieves the goal.

Since I don't think the usage of test_files and extra_rdoc_files is widespread these days, I am definitely in favor of (2).

And how about this condition if "%{-t}%{-r}" == "-t-r"

I had to add additional spec.files -= filenames \ to remove macro, because when adding to test_files file also popped in files.

Not mentioning that this is going to be called twice when neither of the options is specified (of course it won't change result).

Not mentioning that this is going to be called twice when neither of the options is specified (of course it won't change result).

Yes, duplicating the operation was just a quick workaround. In no way I do see it as a solution. (2) sounds reasonable, but couldn't we just move the logic to:

3) every %{-t}%{r} argument specified translates to additional removal, so we can use all of them at once. Meaning that by default, file is remove only from spec.files, but if I specify -t then also from test_files etc..

3) every %{-t}%{r} argument specified translates to additional removal, so we can use all of them at once. Meaning that by default, file is remove only from spec.files, but if I specify -t then also from test_files etc..

I was thinking about this as well, I almost proposed it, but then I realized, that what if you really want to remove just stuff from test_files? There would be no way to do this.

I have also one crazy idea, that instead of these options, we could have lets say "-f 'test_'" if you can follow.

I was thinking about this as well, I almost proposed it, but then I realized, that what if you really want to remove just stuff from test_files? There would be no way to do this.

Well, in that unusual case, one would have to use two commands- 1. to remove it from 'test_files' and 2. to add it to 'files'. I do not see nothing wrong in its explicitness. And you need just one command to remove it from test_files (and from files too; why is it implicitly in files anyway?), which is what one usually intends to do. That's why I do really like (3) and I'd like to see it accepted. From mine POV, if it gets implicitly added to files it should be 'implicitly' removed, too.

So that:

%gemspec_add_file -t 'my_file.txt'
%gemspec_remove_file -t 'my_file.txt'

would be NOOP.

Question is: What would happen if one was to remove it only from files and leave it in test_files? Is the .gemspec inconsistent in that case? Does it get added (again) implicitly, so it's more like NOOP? (I'll test this.)

I have also one crazy idea, that instead of these options, we could have lets say "-f 'test_'" if you can follow.

This seems really rather uncontrolled variant, to me.

I was thinking about this as well, I almost proposed it, but then I realized, that what if you really want to remove just stuff from test_files? There would be no way to do this.

Well, in that unusual case, one would have to use two commands- 1. to remove it from 'test_files'

Honestly, I don't expect that this case is usual at all. In the "unusual" case, where there are some files listed in test_files, it won't harm to run the command twice (although there should be note about this).

and 2. to add it to 'files'. I do not see nothing wrong in its explicitness.

Sorry, but this is awful. If the item is listed in two lists, it is ok to call some command twice to remove it from two lists. However removing the item from two lists just to add it again into one of them makes no sense IMO.

why is it implicitly in files anyway?

I think this is for historic reasons, where it was expected that the test suite will be executed after installation and this never really worked.

if it gets implicitly added to files it should be 'implicitly' removed, too.

If Ruby gems are doing some magic behind, it does not mean we have to do it. Moreover, I don't thing RubyGems are behaving consistently. There for example exists Specification#normalize method, which actually adds the test_files and extra_rdoc_file into files the files, but there are also other places, where just the test_files are added into the files section etc. This whole is mess I would like to stay away from.

So that:
%gemspec_add_file -t 'my_file.txt'
%gemspec_remove_file -t 'my_file.txt'

would be NOOP.

Understand, but I hope nobody will do this anyway.

Question is: What would happen if one was to remove it only from files and leave it in test_files? Is the .gemspec inconsistent in that case? Does it get added (again) implicitly, so it's more like NOOP? (I'll test this.)

I don't think that the situation "item is listed in test_files while not listed in files" is valid nor it is possible to achieve it.

Sorry, but this is awful. If the item is listed in two lists, it is ok to call some command twice to remove it from two lists. However removing the item from two lists just to add it again into one of them makes no sense IMO.

Ok, so it's (2) then. The two removals then have to be specified in correct order(test_files first and then files), right? How about we just add -a option to remove all?


Thanks for answering.

rebased onto a62017d455e877608e9b735f931d6d79e6d54668

a year ago

And how about this condition if "%{-t}%{-r}" == "-t-r"

I quite like the notation 1 < ( 0 %{-t:+1} ... ) more.

How about we just add -a option to remove all?

That make sense. Yep.

And how about this condition if "%{-t}%{-r}" == "-t-r"
I quite like the notation 1 < ( 0 %{-t:+1} ... ) more.

This does things which are not apparent, why is there the 0, why is there the +1, etc. Just check the output in the build log.

Edit all? What all? This is probably not the most expressive description ...

You should remove first the content form extra_rdoc_ and test_ files and then remove conent from the files ...

Other than the syntax, discussed elsewhere, you still not provide any meaningful about message.

How about we just add -a option to remove all?
That make sense. Yep.

Actually, thinking about that once more, I don't support the -a option anymore. The -a option is solving artificial case and just complicates the logic. Please drop the option for now. We can always reintroduce it later.

It also makes moot two of my previous inline comments ...

Actually, thinking about that once more, I don't support the -a option anymore. The -a option is solving artificial case and just complicates the logic. Please drop the option for now. We can always reintroduce it later.

Ok, let's wait for some practical use-case then.

rebased onto 803f84f5690efaa1012c13d20d0b4f0b1681c5d7

a year ago

Still, the abort message might be a bit confusing, since there is also "-s" option, but my original proposal was not really better, so I'm fine with your latest proposal ;)

Otherwise I have not tested the latest version, but it generally LGTM. So please merge it and build Ruby.

It would be also nice to have this backported into all supported Fedoras including these two commits:

https://src.fedoraproject.org/rpms/ruby/c/bee5851fb0985cbe31515cd2addafec081809b7a?branch=master
https://src.fedoraproject.org/rpms/ruby/c/561c76845a4c29e811808e50d259956cbff7ab08?branch=master

This would unblock https://pagure.io/packaging-committee/issue/710

So please merge it and build Ruby.

Unless you really want me to test it ;)

Still, the abort message might be a bit confusing, since there is also "-s" option, but my original proposal was not really better, so I'm fine with your latest proposal ;)

I'll clear that, thanks for noticing.

Otherwise I have not tested the latest version, but it generally LGTM. So please merge it and build Ruby.
It would be also nice to have this backported into all supported Fedoras including these two commits:
https://src.fedoraproject.org/rpms/ruby/c/bee5851fb0985cbe31515cd2addafec081809b7a?branch=master
https://src.fedoraproject.org/rpms/ruby/c/561c76845a4c29e811808e50d259956cbff7ab08?branch=master
This would unblock https://pagure.io/packaging-committee/issue/710

Will do.

Unless you really want me to test it ;)

I'll test this prior to merging and post result here(ASAP).

+  abort("Use only one '-t' or '-r' at a time.") if "%{?-t}%{?-r}" == "-t-r" \

rebased onto 79d93ca818083404c2e48fde2f4b3c79fb62bb8f

a year ago

All tests passed, although, the abort is not much obvious with all the verbosity we have:

DEBUG: + ruby
DEBUG: + echo 'gemspec_file = '\''/builddir/build/BUILD/hiredis-0.6.1.gemspec'\''
DEBUG: BUILDSTDERR:
DEBUG: BUILDSTDERR:   abort("Use only one '\''-t'\'' or '\''-r'\'' at a time.") if "-t-r" == "-t-r"
DEBUG: BUILDSTDERR:
DEBUG: BUILDSTDERR:   filenames = "test_nonexistent/tr"
DEBUG: BUILDSTDERR:   filenames = Array(filenames)
DEBUG: BUILDSTDERR:
DEBUG: BUILDSTDERR:   spec = Gem::Specification.load(gemspec_file)
DEBUG: BUILDSTDERR:   abort("#{gemspec_file} is not accessible.") unless spec
DEBUG: BUILDSTDERR:
DEBUG: BUILDSTDERR:   spec.test_extra_rdoc_files += filenames
DEBUG: BUILDSTDERR:   File.write gemspec_file, spec.to_ruby'
DEBUG: BUILDSTDERR: Use only one '-t' or '-r' at a time.
DEBUG: BUILDSTDERR: error: Bad exit status from /var/tmp/rpm-tmp.HILgMm (%prep)
DEBUG: BUILDSTDERR:     Bad exit status from /var/tmp/rpm-tmp.HILgMm (%prep)
DEBUG: RPM build errors:
DEBUG: Child return code was: 1

I'll at least prepend it with the macro name.

rebased onto 8ef3cc0

a year ago

Pull-Request has been merged by pvalena

a year ago