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

Expose the generated rpm filepath #1287

Closed
Lasering opened this issue Nov 27, 2019 · 8 comments · Fixed by #1299
Closed

Expose the generated rpm filepath #1287

Lasering opened this issue Nov 27, 2019 · 8 comments · Fixed by #1299

Comments

@Lasering
Copy link
Contributor

The generated rpm filename should be exposed via a setting.

I generate an rpm via sbt rpm:packageBin in a CI which I then upload to a nexus using curl:

# This generates the rpm but does not log anything to the console
PACKAGE_OUTPUT=$(sbt "print rpm:packageBin") 
echo $PACKAGE_OUTPUT
# Extract the rpm filename full path
RPM_FILE=$(echo $PACKAGE_OUTPUT | tail -n1)
curl --user "$NEXUS_USERNAME:$NEXUS_PASSWORD" --upload-file $RPM_FILE https://nexus.example.com/repository/yum-releases/7/os/$ARCH/$(basename $RPM_FILE)

But its quite clunky as the sbt "print rpm:packageBin" is what actually generates the rpm. It is also the only way to know the generated rpm filename (apart from computing it from the other settings). The code that would be easy to read should be something like:

sbt "rpm:packageBin"
RPM_FILE=$(sbt "rpm:filename" | tail -n1)
curl --user "$NEXUS_USERNAME:$NEXUS_PASSWORD" --upload-file $RPM_FILE https://nexus.example.com/repository/yum-releases/7/os/$ARCH/$(basename $RPM_FILE)

Information

  • What sbt-native-packager are you using: 1.4.1
  • What sbt version: 1.3.2
  • What is your build system: Arch Linux + rpm-org package
  • What package are you building: rpm
  • What version has your build tool: 4.15.0
  • What is your target system: CentOS 7
@muuki88
Copy link
Contributor

muuki88 commented Nov 28, 2019

Thanks for your feature request 😃

To understand the issue better let me recap

  • You generate the rpm file at some point
  • At a later point you find to find out the file name, but without creating the rpm again

If this is correct then I have a few questions

  • Why not merge the rpm creation and uploading into one as you show in the script above, which is a perfectly valid solution
  • What if things change between rpm:packageBin and rpm:filename ?
  • Why not use the rpm:target directory and grep for the rpm file ?

I'm always very cautions with adding new settings that don't add any value, but act as a workaround to remove some other workaround 😁 Adding settings is easy, removing isn't 😉

@nigredo-tori
Copy link
Collaborator

nigredo-tori commented Nov 28, 2019

Just my two cents here. sbt "print foo", while useful, requires the user to do some post-processing (tail -n1 here), and doesn't scale well (if you have two tasks like that, you will need to run SBT twice). Thus I often find it helpful to generate a file with a known path, without worrying about SBT output, etc.. I think it's reasonable to add the rpm/packageBin/artifactPath setting to customize the output path (not just filename) .

(all of this is a personal opinion of a person who doesn't use the RPM plugin).

@muuki88
Copy link
Contributor

muuki88 commented Nov 29, 2019

With the proper cli params no post processing should be required

$ sbt --error --supershell=false "print version"

returns only the version and nothing else 😎
See sbt/sbt#4341

Thus I often find it helpful to generate a file with a known path, without worrying about SBT output, etc..

I would argue that the RPM path is well known as well. Not the exact file, but the output directory which only contains one file.

It's also totally legit to build some helpers around that suit your needs. From my experience build pipelines can differ quite a lot, so baking a single use case into native-packager seems weird.

What we could do without introducing a new setting is the following hack

Rpm / packageBin / targetDirectory := // define the rpm path
Rpm / packageBin := {
   val rpmFile := (Rpm / packageBin / targetDirectory).value
}

I consider this a hack as directory != file and I would argue that the user intention is that Rpm / targetDirectory == Rpm / packageBin / targetDirectory

@muuki88 muuki88 closed this as completed Nov 29, 2019
@nigredo-tori
Copy link
Collaborator

@muuki88,

returns only the version and nothing else

This also means that we lose most of the log in case anything goes wrong. Also, I don't think this would work if some task logs something at the error level.

I would argue that the RPM path is well known as well. Not the exact file, but the output directory which only contains one file.

  1. The almost known path is not always convenient. E.g. I have a hand-written Dockerfile that I run after a build (I haven't switched to DockerPlugin yet), so without a setting like this I would have to either postprocess the build output to move the file to a known path (easy, but annoying), or pass this uncertainty to the resulting docker image.
  2. There is no guarantee that there is only one file, unless the process is happening in CI. E.g. if I package, change version and package again, I will probably get two files. Again, this can be mitigated by cleaning the output folder before the build - but that is also annoying.

I consider this a hack as directory != file

That, indeed, would be a hack. Which is why we should use artifactPath instead:

Rpm / packageBin / artifactPath := ???
Rpm / packageBin := {
   val rpmFile := (Rpm / packageBin / artifactPath).value
}

This also doesn't require any new settings, but it's as idiomatic as it gets - you'll find that Compile / packageBin uses Compile / packageBin / artifactPath for the same purpose, albeit with a bit of indirection.

@Lasering
Copy link
Contributor Author

Lasering commented Nov 29, 2019

  • Why not merge the rpm creation and uploading into one as you show in the script above, which is a perfectly valid solution

I did it with two separate steps: the packageBin and the curl.

  • What if things change between rpm:packageBin and rpm:filename ?

If packageBin uses the setting rpm:filename to generate a new rpm the final generated file will still be at the location pointed by rpm:filename. If the filename is computed using the other settings (packageArchitecture, version, rpmRelease, etc) then it will always point to the correct file.

  • Why not use the rpm:target directory and grep for the rpm file ?

Because it forces me to consider more failure cases. Given that native-packager already knows the full path to the rpm it is also unnecessary.

Currently the code first invokes buildPackage and only after computes the generated file path using the settings. What I'm suggesting is to create a new setting which computes the file path using the settings before hand, and then just return that setting in the buildRpm function. Or in other words refactor the computation of the generated file to a setting.

I would argue that the RPM path is well known as well. Not the exact file, but the output directory which only contains one file.

That would be bad code. If the plugin changes the code would break as that assumption would no longer be valid. If instead I use a path which the plugin says it used to generate the file then that would always be the right path.

@muuki88
Copy link
Contributor

muuki88 commented Dec 4, 2019

Awesome @nigredo-tori ! I haven't yet come across artifactPath.

@Lasering would you be interested in making a pull request that introduces

Rpm / packageBin / artifactPath := ???
Rpm / packageBin := {
   val rpmFile := (Rpm / packageBin / artifactPath).value
}

as described in @nigredo-tori comment?

@muuki88 muuki88 reopened this Dec 4, 2019
@Lasering
Copy link
Contributor Author

Lasering commented Dec 4, 2019

@muuki88 yes!

@muuki88
Copy link
Contributor

muuki88 commented Dec 4, 2019

Looking forward ❤️

Lasering added a commit to Lasering/sbt-native-packager that referenced this issue Jan 26, 2020
muuki88 added a commit that referenced this issue Jan 30, 2020
* Introduce RPM / packageBin / artifactPath setting. Closes #1287

* Fix setting cannot depend on a task.

* Proper fix.

* Now without breaking mima.

* Add a little bit of documentation. And a sbt-test.

* Fix test

Co-authored-by: Nepomuk Seiler <muuki88@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants