Better RPM spec updates

[strzibny]

Hi,

as a Ruby packages maintainer I really welcome any automation effort and polisher is already so far that I would really like to use it for my semi-automatic spec updates. However, to be able to do this I have to bring polisher and associated update script to the point it matches the Ruby guidelines and works for most of our gems in Fedora. Today I started this effort and I bring the first parts of improvements.

Namely:

1, include license files in the main package (according to the guidelines and most gems already in Fedora) 2, increase the list of possible license files according to what upstream projects use 3, properly mark files as %doc (this is important especially for license files that should be in the main package) 4, prefer packager name coming from rpmdev-packager as this is what RPM packagers likely use (they probably use rpmdev-bumpspec which use the same source) 5, fix update message by including the gem's name 6, add in-place update option to the spec update script so it can be used very easily

Thank you for the consideration, it's a only a start, but I thing the stuff coming in this PR is really essential and common (nothing much to argue about I hope).

[coveralls]

Coverage decreased (-55.75%) when pulling 129ce2382c96f238882650c47547f96034da8cb5 on strzibny:better-spec-updates into 32ee2f32b9d0e7e4f6e34083dfa3ed1d5e1e9aa5 on ManageIQ:master.

[movitto]

@strzibny looked this over, alot of great changes.

The only major comment I had was around the 'PACKAGER' const. I'm all for calling out to rpmdev-packager (actually didn't know about this set of tools, thx for the tip) but don't think we should impose the requirement at the class level, eg whenever someone requires 'polisher/rpm/spec'. Case in point, the travis build is failing since this command isn't available there.

Perhaps we could do something like the following, adding a helper method to simply run rpmdev on first invocation and store the result internally:

def self.packager @packager ||= AwesomeSpawn.run('/usr/bin/rpmdev-packager').stdout.chmop end

def self.current_author ENV['POLISHER_AUTHOR'] || packager || AUTHOR end

Then we can remove the rpmdev-packager from being a hard requirement in the gemspec / stub it out as necessary in the spec suite.

Thoughts? Rest looks great, looking forward to getting it in.

[coveralls]

Coverage decreased (-0.27%) when pulling 3b3d2b7b25df1258ee1f97c110937b77de461986 on strzibny:better-spec-updates into 32ee2f32b9d0e7e4f6e34083dfa3ed1d5e1e9aa5 on ManageIQ:master.

[strzibny]

Hi, I fixed your suggested line and tried it out:

# AwesomeSpawn.run('/usr/bin/rpmdev-packager').stdout.chmop
> AwesomeSpawn.run('/usr/bin/rpmdev-packager2').output.chop
AwesomeSpawn::NoSuchFileError: No such file or directory - /usr/bin/rpmdev-packager2

But if the command is not present, it raises an exception. That's why I went with if system("#{cmd} &> /dev/null"), because on my machine it just returns true/false. What I don't get is why it fails on Travis. I must be missing something obvious. On my machine bundle exec rake succeeds every time. Any idea? I can of course rebase it to use AwesomeSpawn, but then we would either need to adjust AwesomeSpawn or be catching an exception, which is what I tried to avoid.

[jrafanie]

@strzibny Thanks for the pull request.

I'm guessing it's failing on travis because it's running in ubuntu without the rpmdev-packager script. AwesomeSpawn.run will not catch exceptions raised for missing commands.

We should be able to check File.exist?(PACKAGER_SCRIPT) && File.executable?(PACKAGER_SCRIPT) or something similar and only then run the script. If it fails this check, we can fallback to checking some other information such as the environment variables for the packager info or just fail outright with information on how to install the rpmdev-packager script via xyz package in fedora, tarballs for other platforms here, etc.

From what I understand, s.requirements << 'rpmdev-packager' in the gemspec is only a "suggestion" to the consumer that there are external dependencies. We don't actually enforce this dependency is installed.

[movitto]

@jrafanie ah good point re requirements, forgot about the distinction between gemspec requirements and dependencies, we should probably add a note requirement for each / all of the optional dependencies (eg "various polisher modules require the following optional dependencies to work: ___")

RE awesome_spawn, think it would be worth adding a method and / or check to existing method(s) to check for the existence of the command being run? Perhaps an an optional feature that could be toggle via a method arg or similar. The "run command if it exists, else raise error" seems to be a common edge case and might make sense in AwesomeSpawn

[jrafanie]

@strzibny @movitto I'd be fine with using the same convention for "delay" raising exceptions on missing prerequisites for rpmdev-packager as we do for the pkgwat, gem2rpm, etc.

@movitto Are you suggesting we auto catch Errno::ENOENT and raise a different exception? Right now it's raising AwesomeSpawn::NoSuchFileError, I believe. Can you give an example? I'd like to move it to AwesomeSpawn if it makes sense. I'm curious what you're thinking.

[movitto]

@jrafanie Currently the delayed-exception processing of soft-reqs facilitate ruby library 'requires' but not local system commands. It could be expanded to facilitate this, but that increases the complexity / prolly should be in another PR.

Was just suggesting we also had 'requirements' in our spec for the optional dependencies. Like you mentioned these are just suggested (printed out on gem install) but not enforced. Again that can / prolly should be in a seperate PR.

As far as awesome_spawn, it seems like the run method currently does explicitly raise a custom error if the command is not found, perhaps that could just be caught here. I'm a little confused as to why it does this though, usually the non '!' versions of methods do no raise errors while the '!' versions do.

[strzibny]

we should probably add a note requirement for each / all of the optional dependencies (eg "various polisher modules require the following optional dependencies to work: ___")

Yes, that was my idea as well. Because otherwise users of RubyGems.org version can't easily see all the possible requirements before using the gem.

As far as awesome_spawn, it seems like the run method currently does explicitly raise a custom error if the command is not found, perhaps that could just be caught here. I'm a little confused as to why it does this though, usually the non '!' versions of methods do no raise errors while the '!' versions do.

I wanted to avoid catching any exceptions at first, but perhaps it makes sense to just catch/skip the exception here at least for now until awesome_spawn handles 'try this command and (optionally) throw exception' scenario.

[strzibny]

I'm guessing it's failing on travis because it's running in ubuntu without the rpmdev-packager script. AwesomeSpawn.run will not catch exceptions raised for missing commands.

So I went ahead and found out that even though Ubuntu uses bash as default, it links /bin/sh (what Ruby always uses) to dash (Debian POSIX shell) and that is where it fails. But actually it's probably better not to rely solely on bash. I will go and adjust the PR to use awesome_spawn catching the exception.

[strzibny]

Rebased https://github.com/strzibny/polisher/commit/b7410b511ca64c3fb477e2f28604b363c1c7423e. should be working as expected now.

[miq-bot]

Checked commits https://github.com/strzibny/polisher/commit/efac4cd1847732a57f4e1a15beb2ccb2a4e685dc .. https://github.com/strzibny/polisher/commit/9c2ab4ee6eafcb670bb97290f2fe83f5821f8633 with rubocop 0.21.0 5 files checked, 1 offense detected

lib/polisher/rpm/spec.rb

• [ ] Warn - Line 59, Col 9 - HandleExceptions - Do not suppress exceptions.

[coveralls]

Coverage increased (+0.04%) when pulling 9c2ab4ee6eafcb670bb97290f2fe83f5821f8633 on strzibny:better-spec-updates into 32ee2f32b9d0e7e4f6e34083dfa3ed1d5e1e9aa5 on ManageIQ:master.

[jrafanie]

@movitto I like this change. I agree with you that we can change AwesomeSpawn.run if need be, but I believe we thought ENOENT for a no such file or directory was so common that we wanted to handle it natively in AwesomeSpawn.

Maybe @Fryguy @brandondunne remember the rational? Was it just so we don't have to catch ENOENT and then parse the message to check for the "no such file or directory"? See here and here

[Fryguy]

@jrafanie Yes, the rationale was that we didn't want to have to make every caller parse the "no such file or directory" just to get at the information. Note that NoSuchFileError is a subclass of ENOENT, so you can still rescue ENOENT in the calling code and it will still work.

[jrafanie]

Good point @Fryguy regarding subclassing of ENOENT. My google searches indicate ENOENT can be raised for "missing directories or files (or symlinks, or sockets, or pipes, or devices)", but they always seem to be raised with "No such file or directory" in ruby, making the AwesomeSpawn subclass similar to just rescuing ENOENT directly. At least if sockets or pipes or devices raised ENOENT with "No such socket/pipe/device", then it would make some sense to subclass ENOENT for a specific type of exception. But alas, my google skills can't track down an ENOENT with a non-"No such file or directory" message.

Either way, I think we can merge this and look at that in AwesomeSpawn later.

[Fryguy]

@jrafanie I agree...I would not be opposed to just removing the NoSuchFileError in the future, if it's essentially meaningless... @brandondunne might have thoughts on this, but we can take that up in AwesomeSpawn.

[jrafanie]

Thanks @strzibny :star: