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

Update Distribution Spec for Windows vs Linux Buildpackages #148

Merged
merged 2 commits into from
Oct 16, 2020

Conversation

ameyer-pivotal
Copy link
Contributor

@ameyer-pivotal ameyer-pivotal commented Sep 29, 2020

Updates layer expectations for Windows buildpackages

Signed-off-by: Andrew Meyer meyeran@vmware.com


This change accommodates Windows buildpackages, which currently require a base image in order to be pulled into a local docker daemon. This may change in the future, and the clarification to the spec will allow for this flexibility, since it will no longer include any potential base layers in the contract.

Also, regarding the Windows specific blob format (where tar paths must be prefixed with Files/), see the last paragraph of this spec page, as it is a blanket statement that covers all of the spec. Therefore we did not add the prefix to this dist spec specifically.

@ameyer-pivotal ameyer-pivotal requested a review from a team as a code owner September 29, 2020 18:15
@ameyer-pivotal ameyer-pivotal changed the title Update distribution.md Clarify Distribution Spec Sep 29, 2020
distribution.md Outdated Show resolved Hide resolved
distribution.md Outdated Show resolved Hide resolved
Clarify intention by mentioning buildpack layers specifically, so as to not inadvertently comment on format of other non-buildpack layers

Signed-off-by: Andrew Meyer <meyeran@vmware.com>
distribution.md Outdated
@@ -24,7 +24,7 @@ A buildpackage MUST exist as either an OCI image on an image registry, an OCI im

A `.cnb` file MUST be an uncompressed tar archive containing an OCI image. Its file name SHOULD end in `.cnb`.

Each FS layer blob in the buildpackage MUST contain a single buildpack at the following file path:
Each FS layer blob in the buildpackage intended to be a buildpack MUST contain a single buildpack at the following file path:
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this change. Don't we dictate that every layer in a buildpackage MUST be a buildpack. When is there a layer in a buildpackage that is not intended to be a buildpack?

Copy link
Contributor Author

@ameyer-pivotal ameyer-pivotal Oct 5, 2020

Choose a reason for hiding this comment

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

@ekcasey This is described in more detail in the PR description, but the short version is: Windows buildpackages have base/OS layers that are not buildpacks. Even the smallest viable base, though small, is not exactly equivalent to scratch on linux.

Copy link
Member

Choose a reason for hiding this comment

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

@ameyer-pivotal thanks for the clarification, I knew runnable windows images needed some sort of base layer, but I was unaware that even docker pull required one!

Copy link
Member

Choose a reason for hiding this comment

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

I worry that the wording may be confusing to readers without the explicit windows context. Do we want to say something like "Each FS layer blob must contain a single buildpack with the following exception.. . Each buildpack layer MUST...".

Copy link
Contributor Author

@ameyer-pivotal ameyer-pivotal Oct 7, 2020

Choose a reason for hiding this comment

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

@ekcasey What exception would be specified? OS layers? The approach I was taking is to have the spec not specifically mention any layers that aren't in the contract (i.e. not mention non-buildpack layers). Since platforms consuming buildpackages only look at buildpack layers, the author of a buildpackage should technically be able to put whatever they want in any other layers, as they would not be "under contract/spec."

That said, I'm willing to budge if it helps. Perhaps a simple change to use non-OS layer blob instead of FS layer blob in the original wording (though that might bring up ambiguity about what an OS layer is/isn't). This would give Each non-OS layer blob in the buildpackage MUST... Thoughts? The more I re-read that, the more I feel we'd go down a rabbit hole if we start mentioning other layer types. My stance is that the dist spec should ONLY comment on buildpack layers because that's all it cares about.

Copy link
Member

Choose a reason for hiding this comment

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

@ameyer-pivotal can we put this context in your git commit as well? This clarification has been super insightful.

Copy link
Member

Choose a reason for hiding this comment

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

After chatting with @ekcasey @nebhale and @hone, we'd suggest adding an exception for Windows, as opposed to opening up the buildpackage format to arbitrary layers.

Copy link
Contributor Author

@ameyer-pivotal ameyer-pivotal Oct 7, 2020

Choose a reason for hiding this comment

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

@sclevine Actually, the reason for the original approach is that Windows buildpackages may not always look this way. We didn't want to cause a change to the spec if we're ever able to have a scratch equivalent for Windows images in the future (currently, scratch-based images work when packages are remote-only -- it's just pulling with docker that causes the issue). I still think that the spec should comment only on the parts it cares about. How would additional layers impact the platform's ability to look for the layers it's looking for? I.e. what do we gain by being restrictive?

Perhaps I should have gone with an RFC after all, as it seems like there is still some conversation to be had. I had understood the original suggested change to be a simple clarification of the original intention of the spec (all about buildpack layers), but adding exceptions for specific OSes feels like a more substantive change to the spec, rather than a clarification of the original spec.

(cc: @ekcasey @nebhale @hone)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ekcasey @sclevine @jkutner @hone @nebhale Made an update to the wording to make specific callouts for Linux vs Windows packages. After giving it some thought, I'm on board with avoiding opening up buildpackages to arbitrary layers. If Windows buildpackages no longer need OS layers in the future, we can address it then with an RFC if desired.

@ekcasey ekcasey changed the base branch from main to distribution/0.2 October 5, 2020 18:59
@ekcasey ekcasey changed the base branch from distribution/0.2 to main October 5, 2020 19:01
@ekcasey ekcasey added this to the Distribution 0.2 milestone Oct 6, 2020
@ekcasey ekcasey changed the base branch from main to distribution/0.2 October 6, 2020 14:35
@nebhale nebhale requested a review from a team October 7, 2020 18:13
Signed-off-by: Andrew Meyer <meyeran@vmware.com>
@ameyer-pivotal ameyer-pivotal changed the title Clarify Distribution Spec Update Distribution Spec for Windows vs Linux Buildpackages Oct 14, 2020
@nebhale nebhale self-requested a review October 15, 2020 12:23
@jkutner jkutner merged commit d24b1ad into buildpacks:distribution/0.2 Oct 16, 2020
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.

6 participants