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

RFC: System Buildpacks #179

Merged
merged 17 commits into from Mar 4, 2022
Merged

RFC: System Buildpacks #179

merged 17 commits into from Mar 4, 2022

Conversation

jkutner
Copy link
Member

@jkutner jkutner commented Jul 29, 2021

Readable

This RFC supplements #175 and #168

@jkutner jkutner requested a review from a team as a code owner July 29, 2021 03:27
@buildpack-bot
Copy link
Member

buildpack-bot commented Jul 29, 2021

Maintainers,

As you review this RFC please queue up issues to be created using the following commands:

/queue-issue <repo> "<title>" [labels]...
/unqueue-issue <uid>

Issues

@jkutner jkutner changed the title Add RFC for System Buildpacks RFC: System Buildpacks Jul 29, 2021

## Detection

The results of detection by system buildpacks MUST NOT influence the selected buildpack group. If no system buildpacks pass detection, any buildpack group MAY pass detection. If a system buildpack pass detection and no buildpack groups pass detection, then detection MUST fail.
Copy link
Member

Choose a reason for hiding this comment

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

The results of detection by system buildpacks MUST NOT influence the selected buildpack group.

I'm not sure I understand this requirement. If system buildpacks can augment the build plan, they will also influence the selected buildpack group.

If no system buildpacks pass detection, any buildpack group MAY pass detection.

Similar issue to the above -- certain groups may require a system buildpack to include build plan entries.

If a system buildpack pass detection and no buildpack groups pass detection, then detection MUST fail.

Is this just to prevent groups with all optional buildpacks from turning into groups that only contain system buildpacks?

Copy link
Member Author

Choose a reason for hiding this comment

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

The results of detection by system buildpacks MUST NOT influence the selected buildpack group.

I'm not sure I understand this requirement. If system buildpacks can augment the build plan, they will also influence the selected buildpack group.

I really meant for this to be about exit code. With regard to the build plan, maybe the system buildpack should have either not have that, or have a separate build plan.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that I would like to add a +1 to @sclevine point. I don't understand why these wouldn't just be treated as buildpacks that you want to run before and after every other group. Maybe I am missing something bigger here but I feel like the concept to me was simpler if it is essentially just matrix expansion if you will. And with the ability to mark the buildpacks optional I think you still have a very wide range of flexibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm hoping to avoid having our decision to decouple certain behaviors (like .profile) from the implementation of the lifecycle manifest itself in the user experience.

I think buildpack users shouldn't need to understand or care how things like "optional" or groups, or any other buildpack concepts work. They should run pack build (or whatever for their platform) and it just detects a python app, and builds it. Adding buildpacks like .profile or whatever into the build breaks the abstraction provided by pack. (also maybe this decision can be scoped to pack and not lifecycle).

Ultimately though, I think this comes down to how much we decide to push into the system/utility buildpacks. If it's really just .profile in most cases, then I probably don't care as much. But I worry about a situation where it used to be as simple as "the heroku/python buildpack passed detect" to "why are there 8 buildpacks running?" Again I think that comes back to a difference b/w Heroku/Paketo buildpack design.


## Build

System buildpacks that have passed detection will be executed during the build phase. All `pre` buildpacks must execute before the detected buildpack group. All `post` buildpacks must execute after the detected buildpack group. However, the system buildpacks MUST NOT be added to the buildpack group, and their execution may be hidden from the end user.
Copy link
Member

Choose a reason for hiding this comment

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

Why do we want to hide system buildpacks from the user? Shouldn't we be transparent about what features are activated?

the system buildpacks MUST NOT be added to the buildpack group

Not sure I understand what this means.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do we want to hide system buildpacks from the user? Shouldn't we be transparent about what features are activated?

I went back and forth on this. But the fact that they are buildpacks is a bit of an implementation detail. In verbose logging you would want to see this, but in the normal flow of things, people want to see the buildpacks they care about "node, java, etc". This may be a difference in the Heroku/Paketo stance too.

the system buildpacks MUST NOT be added to the buildpack group

Not sure I understand what this means.

This is really about exit code again, and not wanting passing system buildpacks to allow groups with only optional buildpacks that fail to continue. I'll reword

Copy link
Member

Choose a reason for hiding this comment

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

I can see both ways on this. If we're really extracting out spec/lifecycle features we didn't necessarily log that .profile was written.

Copy link
Member

Choose a reason for hiding this comment

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


The results of detection by system buildpacks MUST NOT influence the selected buildpack group. If no system buildpacks pass detection, any buildpack group MAY pass detection. If a system buildpack pass detection and no buildpack groups pass detection, then detection MUST fail.

System buildpacks may require/provide in the build plan following standard buildpack API specification.
Copy link
Member

@hone hone Aug 11, 2021

Choose a reason for hiding this comment

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

Do you have use cases for system buildpacks you're thinking of that would use the Build Plan? I think with how much you want to "hide" system buildpacks if it's bad that they'd influence the other buildpack execution.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no specific examples/use-cases. I only wanted to default to the having them be the "same" as normal buildpacks

Copy link
Member

Choose a reason for hiding this comment

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

I am not so sure on including system buildpacks in the buildplan if they are not interacting with the normal buildpacks in a group. Could we either limit the system buildpacks plan resolution to their respective groups? pre system buildpacks need to match up all their plan entries separately, normal buildpacks separately and post buildpacks separately.

Given that we want to discourage people from using system buildpacks as hidden buildpacks and given that these are mainly supposed to be utility buildpacks - would a simplifying assumption be to remove system buildpacks from contributing to the build plan entirely? And limit them to just detect pass or fail?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could we either limit the system buildpacks plan resolution to their respective groups?

Meaning they have their own plan, distinct from user defined buildpacks? I think that's a good idea.

I'm fine with any of these, but I agree that I should probably be more specific about which behavior we want in the RFC.

Copy link
Member

Choose a reason for hiding this comment

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

Should we just have buildpacks not be part of the buildplan for now if we don't have a use case for it?

@hone
Copy link
Member

hone commented Aug 11, 2021

@jkutner is it out of scope where the system buildpacks come from? They don't live on the registry do they?

@jkutner
Copy link
Member Author

jkutner commented Aug 11, 2021

@hone i suppose it's out of scope, but I could see them on the registry.

@hone hone added the status/blocked RFC is blocked by some other outstanding dependency. label Sep 15, 2021
@hone
Copy link
Member

hone commented Sep 15, 2021

From last WG, this is blocked on the resolution of #175

@jkutner
Copy link
Member Author

jkutner commented Dec 3, 2021

@hone @sclevine please review

Copy link
Member

@hone hone left a comment

Choose a reason for hiding this comment

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

It looks like there are two remaining issues that still need to be resolved before we can move this forward:

@hone hone removed the status/blocked RFC is blocked by some other outstanding dependency. label Dec 8, 2021
@jkutner jkutner requested a review from hone January 20, 2022 14:04
@jkutner
Copy link
Member Author

jkutner commented Jan 20, 2022

@hone please review the stance taken in the text:

system buildpacks' Build Plan

system buildpacks use the build plan as normal buildpacks do

system buildpacks being hidden from user

They "may" be hidden. I think we should leave this up to platforms.

@jkutner
Copy link
Member Author

jkutner commented Jan 27, 2022

From working group:

  • Enforce that system buildpacks are always optional. Remove optional from schema (but this might cause a problem if we ever want to introduce optional, because the default would change)
  • spec changes will require a new .toml or similar in the builder
  • lifecycle will need a new flag --system-buildpacks
  • need to define how this behaves with different platform API that doesn't support (recommend: it should warn)

text/0000-system-buildpacks-in-builder.md Outdated Show resolved Hide resolved
text/0000-system-buildpacks-in-builder.md Outdated Show resolved Hide resolved
text/0000-system-buildpacks-in-builder.md Outdated Show resolved Hide resolved
text/0000-system-buildpacks-in-builder.md Outdated Show resolved Hide resolved
text/0000-system-buildpacks-in-builder.md Outdated Show resolved Hide resolved
text/0000-system-buildpacks-in-builder.md Outdated Show resolved Hide resolved
text/0000-system-buildpacks-in-builder.md Outdated Show resolved Hide resolved
text/0000-system-buildpacks-in-builder.md Outdated Show resolved Hide resolved

## Use the `[[order]]` table

Instead of a new `[system]` table, we could put `pre` and `post` in the `[[order]]` table. However, this could imply that there is a interaction/override/etc between these buildpacks and the `pre`/`post` buildpacks in `project.toml`. But there is not.
Copy link
Member

Choose a reason for hiding this comment

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

I kind of like this alternative builder.toml schema? But it's not a strong preference.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a strong preference

text/0000-system-buildpacks-in-builder.md Outdated Show resolved Hide resolved
jkutner and others added 7 commits February 4, 2022 08:20
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
text/0000-system-buildpacks-in-builder.md Outdated Show resolved Hide resolved
text/0000-system-buildpacks-in-builder.md Outdated Show resolved Hide resolved
text/0000-system-buildpacks-in-builder.md Outdated Show resolved Hide resolved
text/0000-system-buildpacks-in-builder.md Outdated Show resolved Hide resolved
text/0000-system-buildpacks-in-builder.md Outdated Show resolved Hide resolved
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
jkutner and others added 5 commits February 4, 2022 14:52
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Emily Casey <ecasey@vmware.com>
@jkutner
Copy link
Member Author

jkutner commented Feb 4, 2022

Moving this into FCP, closing Feb 11. (next Friday)

@hone
Copy link
Member

hone commented Mar 2, 2022

@buildpacks/platform-contributors @buildpacks/implementation-maintainers are there any issues that should be created for this?

@hone hone assigned ekcasey and unassigned nebhale Mar 2, 2022
@ekcasey
Copy link
Member

ekcasey commented Mar 4, 2022

/queue-issue buildpacks/rfcs "Implement System Buildpacks"

ekcasey added a commit that referenced this pull request Mar 4, 2022
[#179]

Signed-off-by: Emily Casey <ecasey@vmware.com>
@ekcasey ekcasey merged commit 31c14c3 into main Mar 4, 2022
@ekcasey ekcasey deleted the jkutner/system-buildpacks branch March 4, 2022 19:20
@ekcasey ekcasey removed their assignment Mar 4, 2022
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.

None yet

8 participants