Support pre-package commands with arguments

[c0d1ngm0nk3y]

#fixes paketo-buildpacks/npm-install#856

Summary

The create-package command will invoke the pre-package command from the buildpack.toml.

Use Cases

The build script might receive some arguments for multiarch (e.g. here)

Checklist

• I have viewed, signed, and submitted the Contributor License Agreement.
• I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
• I have added an integration test, if necessary.
• I have reviewed the styleguide for guidance on my code quality.
• I'm happy with the commit history on this PR (I have rebased/squashed as needed).

[dmikusa]

Thanks for the PR. I'm on the fence about this. It adds some complexity with the shell parsing library. I think the intent is for that to point to a script, though, so if you needed to set args you'd put those in your script. For paketo buildpacks, we have a scripts/build.sh script that we use for this.

Can you expand on your situation/use case? Is there a reason you couldn't add a wrapper script that includes your custom arguments? The arguments are going to be static in the buildpack.toml, so I don't think it would be any more flexible to have them in the toml file as opposed to a script. Thanks

[c0d1ngm0nk3y]

It adds some complexity with the shell parsing library.

Can you maybe better explain where you see the complexity. Without using a library, I would totally see your point. But we use the same library as paketo-buildpacks/procfile in the exact same way.

I think the intent is for that to point to a script

But being able to call a script only without arguments, sounds kinda like a bug. Doesn't it?

so if you needed to set args you'd put those in your script. For paketo buildpacks, we have a scripts/build.sh script that we use for this.

That would be an easy workaround for sure. But npm-install decided to add arguments to build.sh. Which I prefer tbh, because otherwise you would hardcode the targets in 2 places - buildpack.toml and build.sh

@pacostas Any thoughts on this?

Can you expand on your situation/use case?

It is all in the linked issue, but basically it is as simple as being able to call create-package for the npm-install buildpack.

Is there a reason you couldn't add a wrapper script that includes your custom arguments?

They are not really custom arguments, but the way the build.sh works for this buildpack.

The arguments are going to be static in the buildpack.toml, so I don't think it would be any more flexible to have them in the toml file as opposed to a script.

But what if I want to add a target or (more likely) remove one for local use. I would have to touch 2 places. Right?

[dmikusa]

Can you maybe better explain where you see the complexity. Without using a library, I would totally see your point. But we use the same library as paketo-buildpacks/procfile in the exact same way.

Shell parsing is a dangerous task, IMHO. It seems simple on the surface but has a lot of corner cases and risk. I think it's something we should avoid attempting to do if there are other options. The procfile buildpack doesn't really have a choice since the Procfile format is something out of our control. I agree with what the buildpack spec did with getting rid of direct=false though. It's far better to have the user to split command and args, where possible.

I also have doubts about the library, as it is not actively maintained. If we need it, like Procfile, we don't have a lot of choice, as there's not another library in Go that I've seen. It's a risk using it, though. If we don't need to, then I'd rather avoid it.

But being able to call a script only without arguments, sounds kinda like a bug. Doesn't it?

It's perspective, and I can see yours, but it depends on intent and what's documented. There was never intent for this to work that way, nor is it documented as working that way either, so the fact that it doesn't work that way is not a bug. It just doesn't work that way.

It is all in the linked issue, but basically it is as simple as being able to call create-package for the npm-install buildpack.

This would be another area where there's some gray area. The create-package tool is meant to be used with libpak buildpacks, and jam is meant to be used with packit buildpacks. If you can use create-package to build packit based buildpacks, that's great, but it's not a scenario that libpak tests or guarantees.

Using jam to build this seems like it would be the best path, which is what I suspect the authors of the buildpack test and do.

I recognize that this is not a great user experience, having two packaging tools, but it's where we are at right now. Hopefully, it won't always be that way.

[c0d1ngm0nk3y]

Shell parsing is a dangerous task, IMHO. It seems simple on the surface but has a lot of corner cases and risk.

I wouldn't call it dangerous. The worst think that can happen is that it dos not work. But it is not simple for sure. Even if it only works for 80% of the cases, that is 80% more as without.

I think it's something we should avoid attempting to do if there are other options.

An option could be to shell out and let the shell take care of it. Do you think that might be an option?

If you can use create-package to build packit based buildpacks, that's great, but it's not a scenario that libpak tests or guarantees.

It is really a coincidence that this buildpack is packit based. The use of create-package has nothing to do with the library and it worked before. To split the tooling for the community based on the library sounds like an odd choice to me tbh. I always use create-package - if I now have to check the used library first, that is odd. Specifically since there is no technical reason.