search

paketo-buildpacks / packit

Buildpacks Utils Library
Apache License 2.0
41 stars 25 forks source link

Retain `strip-components` in `jam update-dependencies` #184

Closed sophiewigmore closed 3 years ago

sophiewigmore commented 3 years ago

Context

PR #183 added support for supplying a strip-components in a buildpack.toml, and PR #181 added a jam update-dependencies command that keeps buildpack.toml dependencies up to date.

Issue

Currently, the jam update-dependencies command and the associated cargo.ConfigMetadataDependency type do not take the strip-components field into account, so when a buildpack.toml file is updated with that jam command, this field will be omitted. This isn't an issue for our Paketo buildpacks since the default strip-components value is 0. However, this will be an issue or any users that want to leverage the jam update-dependencies command for buildpacks that are tar’d in a slightly different manner than our Paketo buildpacks.

Desired Outcome

This issue is to support the strip-components field within the jam update-dependencies command so that it doesn't get erased when updating dependencies.

sambhav commented 3 years ago

I will take a look at this later this week :)

sophiewigmore commented 3 years ago

Update from looking into this issue:

jam update-dependencies doesn't take into account the dependencies in the buildpack.toml right now, but instead it just uses metadata.dependency-constraints to completely re-generate the file. Because of this, it becomes messy to keep track of the pre-existent strip-components field on dependencies. It gets even messier if there are different strip-components for different versions of the same dependency. It's not impossible, just messy and possibly not worth it.

There are a couple paths forward here:

  1. Add an optionalstrip-components field to the metadata.dependency-constraints structure so that it will be added on a dependency and version constraint level when we regenerate the buildpack.toml.

The drawback to this solution is that strip-components doesn't have anything to do with a dependency constraint, so it might be unintuitive or confusing to a user to add it to their constraint section.

  1. Add the strip-components field to the dep-server dependency metadata itself.

The drawback to this solution is that if a buildpack author wants their own custom strip-components value different than what we ship, then we will have the same overwriting problem we have now.

  1. Do nothing for now. The addition of strip-components in PR #183 by @samj1912 is ideally for buildpack authors who may want to use artifacts that are tar’d in a slightly different manner than the dependencies we ship. This use case tells me that a user who wants to set this value probably isn't using our Paketo dep-server dependencies, and is probably not going to use the jam update-dependencies command, unless they set up their own dependency server API to use.

The drawback to this is that if a user isn't aware of this bug and tries this use case, their strip-components field will get lost.

Takeaway

Option 3 is the most enticing right now because it's not clear that this use case is there. For now, I think the best solution is to do nothing unless we hear from a user that this is actually something they would want. @samj1912, as the author of the strip-components PR, do you have any thoughts on this?

sambhav commented 3 years ago

Option 3 is completely fine with me. I think the java buildpacks do have to strip out components in same cases like https://github.com/paketo-buildpacks/adopt-openjdk but I guess that uses a completely different packaging tool.

ForestEckhardt commented 3 years ago

With @samj1912 answer I am going to close this issue. If there is any further development feel free to open a new issue or reopen this issue.