eclipse-archived / packagedrone

Eclipse Package Drone
http://eclipse.org/package-drone
Eclipse Public License 1.0
66 stars 39 forks source link

Support recommended and suggested dependencies #127

Closed Nadahar closed 6 years ago

Nadahar commented 6 years ago

I've trying to make support for recommended and suggested dependencies in rpm-builder. Since it uses org.eclipse.packagedrone.utils.rpm to create the actual RPM package, some changes are needed here as well.

I'm not able to build the full bundle because of several Maven errors that I didn't feel like sorting out for such a small change. I was able to build just org.eclipse.packagedrone.utils.rpm using a "mock-up" POM and use it by replacing the dependency in the local Maven repository, so I have been able to make an RPM and tested it on Fedora 28 and Centos 7. I don't think yum in Centos 7 supports recommended dependencies, so they are simply ignored there. The package installs fine though. On Fedora 28 the recommended packages are installed if available and the suggested packages are ignored. Recommended packages are also uninstalled when the RPM in uninstalled. Ergo, it seems to work as intended.

Since I haven't gotten the whole bundle to build and don't know how this project is organized, I haven't made any tests. This might be required, but I will need further information in that case.

I'm not knowledgeable enough about the inner workings of RPM to fully understand when rpmlib(PayloadFilesHavePrefix) and rpmlib(CompressedFileNames) should be included, but I opted to use them for both recommended and suggested dependencies.

I tried to sign up at the Eclipse website with my GitHub noreply e-mail address which I use when committing, but it seems that e-mail confirmation is required because I'm not able to log in with that account. As such I understand that you can't use my commit, but since this is such a small amount of code I was hoping that some of you could simply commit it instead if you want to use it. I don't have a "public" e-mail address where spam is acceptable, so it would be very inconvenient for me to comply with the Eclipse requirements. I see these changes as so minor that any copyright on them is irrelevant from my point of view anyway.

Sami32 commented 6 years ago

AFAIK, it is available on Fedora >=24 and SUSE >=10 for now, but it will be more widely supported as soon as the others distros will upgrade to RPM >= 4.13 (or perhaps it was 4.12)

EDIT: So probably OpenSUSE support it as well.

ctron commented 6 years ago

Thx @Nadahar for the PR!!

You should be able to build the project by executing the following from the root of the repository:

mvn install -f secondary/pom.xml -Dgpg.skip

I understand that the build might seem overly complex for the RPM functionality, but this is only a small sub-set of the package drone project. I am currently trying to resolve this.

I think you PR looks good there are two formal things which are necessary:

I would also like to suggest one more change. As you mentioned this requires RPM 4.13+. So, looking at the Maven plugin, I would like to have some kind of safeguard that people not just use this and then fail with it, not knowing what the cause is.

My proposal would be to introduce a "RPM compatibility" check which is set to e.g. "4.11" by default (could be an enum). Which you can raise to "4.13". When your new functionality is triggered you do a check like (ensureMinimumVersion(RpmVersions.Version4_13)). Throwing a special (e.g. RpmVersionMismatchException) but unchecked exception containing both version information. This could then be used by the maven plugin to provide a RPM version override and provide a meaningful error message to the user.

Nadahar commented 6 years ago

Thank you @ctron for the feedback.

Regarding the Eclipse Contributor Agreement and the copyright headers: As I tried to explain in my initial comment, my problem isn't with the Eclipse Contributor Agreement but with registering an Eclipse account. The reason is that my e-mail address will be public, and that there's nothing I can do about it.

I've had to abandon my "main" e-mail address many years ago because I used it in public domain registration records. The amount of spam I received grew and grew, and when it reached approximately 1000 spam e-mails a day and I spent more than 2 hours each day just to filter out the "legit" e-mails, I had to give it up. Ditching an e-mail address you have used for years and registered hundreds of places is a major inconvenience and not something I'll risk having to do again.

I understand that the problem can't be that bad today, or none of you would accept the have your e-mail available in public, but I'm simply not willing to risk another spam problem. I currently enjoy receiving no spam at all, and I see that as a confirmation that never disclosing my e-mail in public is the right choice.

I do have an e-mail address that I use for forced registrations, which I never read and simply use as a dumping place for spam. I could theoretically register with this address, but it would undermine the purpose of the e-mail address as I see it, as nobody would be able to reach me using this address. As such, I don't see how to sign up for an Eclipse account without breaking either mine or Eclipse's principles.

This is why I hoped that we could find another solution for such a small contribution. I do not care about having any copyright for this, and would be happy if somebody else could commit it as their code.

When it comes to the compatibility check, I'm not sure I understand. As far as I can understand, this being a pure Java implementation, the RPM can be built independent from any RPM system that might exist on the build system. I can build RPMs using this library on Ubuntu without having any trace of RPM installed. Thus, I don't understand how the build could fail because of compatibility issues.

I've tested the finished RPM on Centos 7, which doesn't support "recommended" and "suggested", and the package installs without any errors. The only consequence that I can see is that "recommended" and "suggested" dependencies are ignored. Since these are optional dependencies in the first place, and there's no way to achieve this on older versions of RPM, I consider this a good outcome.

I might completely have missed something here, and if so: please enlighten me :wink:

Sami32 commented 6 years ago

I can confirm that these dependencies shouldn't need any specific tests, as old RPM resolver will just ignore them without any error, as a way to assure compatibility among the different versions..

It is what they call "weak" and "very weak" dependencies: http://rpm.org/user_doc/dependencies.html

Nadahar commented 6 years ago

I just did some further tests to confirm my understanding of the compatibility situation: I built RPM packages both on Windows 7 and on Arch Linux, none of which have any knowledge about RPM. I then transferred the RPM's to Fedora and tested both packages, and they both work as expected.

The reason I think it works like this is the RpmTags. I assume that RPM is made so that it simply ignores unknown tags. For RPM versions below 4.13 these tags are unknown, so the tags and their payload are ignored and the RPM is installed without taking them into consideration.

This is the only explanation I can find that matches the behavior seen when installing these on Centos 7 (which doesn't support these tags). I don't have any older RPM capable VMs to test on, but I think it's reasonable to assume that the same would be true for earlier versions as well.

ctron commented 6 years ago

This is why I hoped that we could find another solution for such a small contribution. I do not care about having any copyright for this, and would be happy if somebody else could commit it as their code.

I am sorry, but there is no way around this. An agreement has to be signed, otherwise I am not allowed to merge the code. You can always contact "emo@eclipse.org" or "legal@eclipse.org" and try to resolve this issue in another way.

Regarding compatibility I do know that you can build those RPMs on any platform. But you can't use those features on any platform. So adding the flags to your specific RPM will also require this RPM version to be present on the target system. Otherwise those features won't work. I guess this also means that the rpmlib dependencies are not correct.

But in any case, what I want to prevent is that someone makes use of those flags and then later finds out that they aren't supported on the target system (not the build system). Putting in some kind of "Minimum RPM version" would allow the maven plugin to validate if this during the build time and would allow raising an exception (during build time) … enforcing an dedicated decision that this (newer) RPM feature should be used.

Nadahar commented 6 years ago

@ctron I finally understand what you mean :+1:, except for this:

I guess this also means that the rpmlib dependencies are not correct

The reason I considered it "acceptable" that these don't work on a platform with a too old version of RPM, is the nature of these in the first place. Suggested dependencies is just informational in any case, they aren't actually installed. Recommended dependencies are installed, but only if the user doesn't explicitly prevent it and only if they can be resolved. If they can't be resolved (if the package is in EPEL or RPM Fusion, and those haven't been added as repositories to the system), they will be also simply be ignored silently.

That means that it can't be a problem if these aren't installed in the first place, or the dependency in question would have to be made required. These are merely ways to help the user get/be aware of additional packages that will compliment the package being installed in some way.

I'm always in favor of taking compatibility into account though, I think things would work much better if developers made a little bit more effort with this. The problem is that I don't quite see how to make it properly without make a big project out of it. Features are added continuously to RPM, and just by taking a quick look here I can see that there were significant changes to functionality 4.9, 4.11, 4.12, 4.13 and 4.14. I'm sure the same is true if you look further back and in the future. My worry is that if we introduce a "compatibility level" setting, we would give the plugin user the impression that this takes all of this into account, not only recommended and suggested dependencies. Unless all of this is implemented and maintained in the future, i think it would merely be a "false sense of security".

I also think it's problematic to decide on a "default" compatibility level. You suggest 4.11 now, and I assume that has to do with the current version in Centos/RHEL 7? When should this change, and will it be maintained? If the default follows Centos/RHEL 7, the packages might still be incompatible with e.g Centos/RHEL 6 or other distros. As long as the plugin user relies on the default (doesn't know about it and doesn't specify the RPM version), incompatible RPM's can still be created.

My question is if it's not better to leave all this to be between the plugin user and the RPM documentation instead of trying to make something to address a specific situation that can/will lead the plugin user into a false sense of security?

ctron commented 6 years ago

I do agree that the current situation is not perfect. But I don't really want to add anything on top of it. Whatever the default setting might be (that can indeed be something newer than 4.11), I think there should be some kind of validation, preventing the user form creating something which won't work.

Because what will happen is that the maven plugin gets blamed of creating RPM which "don't work".

I also do understand that for this change the impact of "not working" would be rather minimal. But looking at Boolean dependencies it looks different I think.

I could imagine that this would also work the other way round. The RPM library could simply bump the internal "required RPM version" and the Maven plugin could then fetch and validate after the build. Allowing to set validation to "ignore", "warn", "fail". Something like that.

Nadahar commented 6 years ago

I understand your worry, but I'm not sure agree that this is the responsibility of RpmBuilder or rpm-builder. Aren't the dependencies themselves just simple strings that's embedded in the RPM package? I mean, if somebody opts to use Boolean dependencies for example, would this builder even be aware? Or are you thinking that a parser should be made to detect this and verify the general syntax of the dependency fields?

I could imagine that this would also work the other way round. The RPM library could simply bump the internal "required RPM version" and the Maven plugin could then fetch and validate after the build. Allowing to set validation to "ignore", "warn", "fail". Something like that.

Here you lose me. When you say RPM library, are you then thinking of the RPM version installed on the build computer? The RPM packages might very well be built on computers without RPM installed at all.

Edit: Maybe this is something that could be handled using something similar to rpmlib(PayloadFilesHavePrefix)", "4.0-1. It's not entirely clear to me what this does, but it seems to be related to compatibility.

Nadahar commented 6 years ago

Running rpm --showrc on Fedora 28 (RPM 4.14.1) gives:

Features supported by rpmlib:
    rpmlib(BuiltinLuaScripts) = 4.2.2-1
    rpmlib(CompressedFileNames) = 3.0.4-1
    rpmlib(ConcurrentAccess) = 4.1-1
    rpmlib(ExplicitPackageProvide) = 4.0-1
    rpmlib(FileCaps) = 4.6.1-1
    rpmlib(FileDigests) = 4.6.0-1
    rpmlib(HeaderLoadSortsTags) = 4.0.1-1
    rpmlib(LargeFiles) = 4.12.0-1
    rpmlib(PartialHardlinkSets) = 4.0.4-1
    rpmlib(PayloadFilesHavePrefix) = 4.0-1
    rpmlib(PayloadIsBzip2) = 3.0.5-1
    rpmlib(PayloadIsLzma) = 4.4.2-1
    rpmlib(PayloadIsXz) = 5.2-1
    rpmlib(PayloadIsZstd) = 5.4.18-1
    rpmlib(RichDependencies) = 4.12.0-1
    rpmlib(ScriptletExpansion) = 4.9.0-1
    rpmlib(ScriptletInterpreterArgs) = 4.0.3-1
    rpmlib(TildeInVersions) = 4.10.0-1
    rpmlib(VersionedDependencies) = 3.0.3-1

On Centos 7 (RPM 4.11.3) it gives:

Features supported by rpmlib:
    rpmlib(BuiltinLuaScripts) = 4.2.2-1
    rpmlib(CompressedFileNames) = 3.0.4-1
    rpmlib(ConcurrentAccess) = 4.1-1
    rpmlib(ExplicitPackageProvide) = 4.0-1
    rpmlib(FileCaps) = 4.6.1-1
    rpmlib(FileDigests) = 4.6.0-1
    rpmlib(HeaderLoadSortsTags) = 4.0.1-1
    rpmlib(PartialHardlinkSets) = 4.0.4-1
    rpmlib(PayloadFilesHavePrefix) = 4.0-1
    rpmlib(PayloadIsBzip2) = 3.0.5-1
    rpmlib(PayloadIsLzma) = 4.4.2-1
    rpmlib(PayloadIsXz) = 5.2-1
    rpmlib(ScriptletExpansion) = 4.9.0-1
    rpmlib(ScriptletInterpreterArgs) = 4.0.3-1
    rpmlib(TildeInVersions) = 4.10.0-1

This seems to be defined here and from what I understand, RichDependencies is the designation for Boolean dependencies - which wouldbe what we're after, although I'm confused by it specifying 4.12.0-1 - as I thought it should be 4.13.

ctron commented 6 years ago

I understand your worry, but I'm not sure agree that this is responsibility of RpmBuilder or rpm-builder.

I really think it is.

Here you lose me. When you say RPM library, are you then thinking of the RPM version installed on the build computer? The RPM packages might very well be built on computers without RPM installed at all.

I mean the RPM library in this project. I am aware that the code is written in pure Java and can run on any platform. I think that is the main purpose of it.

Nadahar commented 6 years ago

I mean the RPM library in this project. I am aware that the code is written in pure Java and can run on any platform. I think that is the main purpose of it.

Of course you're aware of that, I was only trying to explain why I didn't understand what you meant.

When I created this PR I only focused on what was missing to make it possible to build RPMs with recommended and suggested dependencies. When I look at the project again, I think this is broader than I assumed. It seems like this project can do more than just build RPMs, and that these capabilities should be added in more places. I also see that for example RpmVersion is already implemented, although I have no idea what it is used for.

I'm afraid I'm unable to implement what you ask simply from lack of knowledge of this project and its uses. I often see the same when PRs are submitted to one of the projects I'm involved in. For all the good intentions of the submitter, he/she simply lacks the bigger picture needed to understand all implications.

There must be something essential I'm missing, because I can't see how to implement what you're asking in a meaningful way. As I see it, I must either be given the information I need to get the necessary context (via description or links to documentation), or somebody with the necessary knowledge will have to implement this.

ctron commented 6 years ago

What I would like is a feature, which works in combination with the RPM maven plugin, which allows a developer (user of the plugin) to ensure that the features he is using will be supported by his target system. If his target system is RPM 4.11, then he should have the possibility to specify that and the maven plugin should be able to fail if features are being used which would not be supported by that version. And I don't expect this feature to take into account existing functionality, only for the new functionality you are proposing. And I am open to suggestions on how to get there.

Nadahar commented 6 years ago

I understand what you want, I just don't see a generic solution that will actually achieve this that doesn't require a lot of maintenance and in-depth knowledge of the RPM project . If a specialized solution just for recommended and suggested dependencies is sought, I don't quite see the point as the consequences are minimal. I see it as the authors of RPM feels the same way, since they haven't made a "supported feature tag" for this, but have for many other things like Boolean dependencies.

My problem with seeing a "reasonable" implementation of the compatibility feature combined with what I see as the unreasonable requirement to disclose my e-mail have made me decide to close this PR. There are other "inconveniences" as well weighing in, although I knew about them from the start. The sum of these things simply makes it too much for me. The other "inconveniences" I'm talking about is the fact that this requires Java 8 (we still stick to Java 7 compatibility in general) and that the coding style is very different from what I'm used to which means writing code is very slow as I have to find relevant code to see how it should be formatted.

At least I've shown how little it takes to make this work. Hopefully this can be an inspiration for anyone else that might want to implement this.