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.

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

support RPM 'Prefix'. #242

Merged
merged 3 commits into from
Jun 6, 2014
Merged

support RPM 'Prefix'. #242

merged 3 commits into from
Jun 6, 2014

Conversation

jayaras
Copy link

@jayaras jayaras commented May 6, 2014

This lets you set the Prefix: meta info in the event you want to create a relocatable package.

@@ -11,6 +11,7 @@ trait RpmKeys {
val rpmVendor = SettingKey[String]("rpm-vendor", "Name of the vendor for this RPM.")
val rpmOs = SettingKey[String]("rpm-os", "Name of the os for this RPM.")
val rpmRelease = SettingKey[String]("rpm-release", "Special release number for this rpm (vs. the software).")
val rpmPrefix = SettingKey[Option[String]]("rpm-prefix", "File system prefix for relocatable package.")
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't there need to be a default value for this in the acrchetypes as well?

Copy link
Author

Choose a reason for hiding this comment

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

Maybe? Prefix: is not a required field what would you think it would be?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with the prefix option. What's its purpose?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jayaras is right. By default this should be None as this field is optional and only for special use cases.

@jsuereth
Copy link
Member

jsuereth commented May 9, 2014

I'm all for adding the ability to have a prefix, but for RPMs we autogenerate, shouldn't we be taking it into account?

@jayaras
Copy link
Author

jayaras commented May 9, 2014

@jsuereth What do you mean exactly? Perhaps there is a better approach to handle the situation where one uses prefix in general as I had to make several changes to various parts to make it play nice.

-j

@jsuereth
Copy link
Member

jsuereth commented May 9, 2014

Guess I'd like to see the changes. I feel out of the box would make sense.
On May 9, 2014 1:55 PM, "jayaras" [email protected] wrote:

@jsuereth https://github.com/jsuereth What do you mean exactly? Perhaps
there is a better approach to handle the situation where one uses prefix in
general as I had to make several changes to various parts to make it play
nice.

-j


Reply to this email directly or view it on GitHubhttps://github.com//pull/242#issuecomment-42694750
.

@jayaras
Copy link
Author

jayaras commented May 9, 2014

https://gist.github.com/jayaras/7ad6fe2331a427db589a has what i've changed to make it play nice.

@muuki88
Copy link
Contributor

muuki88 commented May 10, 2014

I had some time to read about the prefix tag. Is this package relocation useful? A good default would be None as this destroys most of the default options (like the /usr/bin stuff).

IMHO it's nice to have this option, but a documentation section is definitely needed if people want to use this. The gist is perfect starting point.

Long comment, short fazit. Code looks fine, documentation would be awesome.

Small manual for the docs:

  1. Install sphinx and sphinx-bootstrap theme
  2. start sbt ~sphinx:generateHtml and open the index.html in the target directory.

@jayaras
Copy link
Author

jayaras commented May 12, 2014

I don't think its a common use case. The rpm.org docs on it have an odd example of moving a command line cd player around.

My use case is mostly this is the legacy way we roll packages here. I think the only people who want to use this are going to be doing something not so standard like I am.

thanks I'll work on some docs in the coming days

@muuki88 muuki88 added this to the 0.8.0 - windows services milestone May 17, 2014
@jayaras
Copy link
Author

jayaras commented Jun 5, 2014

added some info on the docs are we safe to merge?

@muuki88
Copy link
Contributor

muuki88 commented Jun 5, 2014

Awesome. As this is turned off by default it should be fine. If you have time to write a small test this would be even better ;)

@muuki88
Copy link
Contributor

muuki88 commented Jun 6, 2014

@kardapoltsev , if you got time can you check on this? Code looks good to me, but additional verification from you wouldn't hurt :)

@kardapoltsev
Copy link
Member

I'm not very familiar with rpm but code looks good to me too. Nice work, @jayaras )

@muuki88
Copy link
Contributor

muuki88 commented Jun 6, 2014

Works on my machine. We should add a test at somepoint for this.

muuki88 added a commit that referenced this pull request Jun 6, 2014
@muuki88 muuki88 merged commit 03473a6 into sbt:master Jun 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants