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.

Just FYI:

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

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.

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.

And #extra_rdoc_files

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.

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.

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.

https://gist.github.com/pvalena/3d284d067d489454182ab4c4624fa981

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" \

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.