Unify `Dockerfile` naming and simplify + combine scripts

[MarijnS95]

@rpavlik For this to work I should either modify the azure pipeline, or bump to GitHub Actions directly. What would you prefer? I'm personally a fan of doing one change at a time (for reviewability and quick PR throughput), and don't think updating Azure is too bad :)

[MarijnS95]

It looks like the Azure pipeline stopped triggering now that I modified the script (perhaps out of safety concerns), curious how it runs since I went for an array under tags: whereas code snippets suggest to oddly use a multiline codeblock instead.

[oddhack]

Leaving aside the Rust image part, I don't understand what this PR is about - could you explain what the Azure stuff is doing, so I can get my head around this?

As far as the vulkan / asciidoctor-spec images are concerned, I'm unsure of the benefit of having dated / versioned images? The current setup in vulkan with a static container name whose contents are occasionally manually updated allows bumping the asciidoctor version globally across all active branches running CI at once, which is a net win IME though also has potential downsides.

[MarijnS95]

Leaving aside the Rust image part, I don't understand what this PR is about - could you explain what the Azure stuff is doing, so I can get my head around this?

This PR specifically simplifies some scripts, makes the naming across dockerfiles consistent, and uniformly tags all images with both -latest and -<date> (hardcoded date) suffix.

I files this in a hurry as draft to show @rpavlik what I was working on, but didn't get to writing a nice overview even after "finishing" it and marking as ready for review. Is that something you'd like in the commit message and PR description before going further with this?

Azure is the CI, building the containers for testing and publishing them automatically on a push to master.

As far as the vulkan / asciidoctor-spec images are concerned, I'm unsure of the benefit of having dated / versioned images? The current setup in vulkan with a static container name whose contents are occasionally manually updated allows bumping the asciidoctor version globally across all active branches running CI at once, which is a net win IME though also has potential downsides.

Same thought here, we can publish (and republish) those tags but never use them, just always stick to -latest (or would you want to omit that suffix entirely, because we're already using tags to identify the various Khronos-related docker images?). Or do you rather not have those tags at all?

[oddhack]

Leaving aside the Rust image part, I don't understand what this PR is about - could you explain what the Azure stuff is doing, so I can get my head around this?

This PR specifically simplifies some scripts, makes the naming across dockerfiles consistent, and uniformly tags all images with both -latest and -<date> (hardcoded date) suffix.

I files this in a hurry as draft to show @rpavlik what I was working on, but didn't get to writing a nice overview even after "finishing" it and marking as ready for review. Is that something you'd like in the commit message and PR description before going further with this?

Not needed, now that I understand the purpose. But I am not entirely certain that automatically updating the images on merge to the default branch is desirable behavior for my images. I've just been building locally and testing out of the local docker image cache before manually pushing to dockerhub, which should produce identical images - and yet I have a feeling of more control doing it this way. If the Dockerfiles were getting updated frequently automating the build/push process would make more sense, but it's been at most a couple of times/year for the Vulkan images.

Same thought here, we can publish (and republish) those tags but never use them, just always stick to -latest (or would you want to omit that suffix entirely, because we're already using tags to identify the various Khronos-related docker images?). Or do you rather not have those tags at all?

Changing from 'asciidoctor-spec' to '...spec-latest' can get reflected in the vulkan repo default branches immediately, but it takes a while (possibly until just before merger to main) to reflect in the many development branches, so existing branches would tend to stick on the older non-latest image. So I'd like to continue with the current naming scheme for that and the vulkan- images. @rpavlik has different considerations for the openxr- images based on their needs AFAIK, so that's not a general statement.

I may also be a little confused on the difference between image 'name' and 'tag'. Having extra asciidoctor-spec-latest/asciidoctor-spec-date tags as aliases of the existing asciidoctor-spec doesn't affect our uses at all.

[MarijnS95]

Not needed, now that I understand the purpose. But I am not entirely certain that automatically updating the images on merge to the default branch is desirable behavior for my images.

Original behaviour is retained to only do this for OpenXR (other images aren't build-tested either). I can remove it from the Rust PR too - but we should probably at least build-test all the other images?

Changing from 'asciidoctor-spec' to '...spec-latest' can get reflected in the vulkan repo default branches immediately, but it takes a while (possibly until just before merger to main) to reflect in the many development branches, so existing branches would tend to stick on the older non-latest image. So I'd like to continue with the current naming scheme for that and the vulkan- images. @rpavlik has different considerations for the openxr- images based on their needs AFAIK, so that's not a general statement.

It is an odd format as well, since theoretically all these image names should be part of the "repo" bit, and the tag is typically just a combination of versions that this was built on/from/for (usually with a bunch of tags aliasing the same image).

I may also be a little confused on the difference between image 'name' and 'tag'. Having extra asciidoctor-spec-latest/asciidoctor-spec-date tags as aliases of the existing asciidoctor-spec doesn't affect our uses at all.

Yeah exactly, perhaps I should revert part of the changes not emit these unused suffixes for the non-OpenXR images? Or perhaps they can be dropped across the board once OpenXR builds figure out how to automatically refresh the tags on every build?

Depends on how big of an issue breaking changes are going to be though, if it's not possible to affix oneself at a specific version and only bump it together with eventual fixups in CI scripts.

[rpavlik]

Yeah, some of this is more awkward because we have unrelated images in the "repo" part of the name, differentiated only by tag. I don't remember why I did this at first, possibly there was some account quota thing on docker hub?

[rpavlik]

@rpavlik For this to work I should either modify the azure pipeline, or bump to GitHub Actions directly. What would you prefer? I'm personally a fan of doing one change at a time (for reviewability and quick PR throughput), and don't think updating Azure is too bad :)

re azure vs github actions: For most things I'd honestly prefer github actions now that they're in a usable state. They weren't when I started this work.

I also agree it's probably not a big deal to drop the "push" part from CI, it never worked right for me anyway and pushing from local is not that big of a deal and probably gives more control.

[MarijnS95]

Yeah, some of this is more awkward because we have unrelated images in the "repo" part of the name, differentiated only by tag. I don't remember why I did this at first, possibly there was some account quota thing on docker hub?

Can you look that up @rpavlik? IMO it'd be cleaner if we could drop the docker-images bit from khronosgroup/docker-images (that's implied...) and make this khronosgroup/asciidoctor-spec:latest etc :)

re azure vs github actions: For most things I'd honestly prefer github actions now that they're in a usable state. They weren't when I started this work.

Same thought, it'll be simpler and it can be tested on forks.

https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-Readme.md: Docker is installed so we don't even need to use the special docker actions in the CI script, but can call straight into the shell scripts created here. That way those are neatly reused and tested at the same time.

I also agree it's probably not a big deal to drop the "push" part from CI, it never worked right for me anyway and pushing from local is not that big of a deal and probably gives more control.

Shall we do that in a separate followup PR though? Don't want to conflate this one :)

[MarijnS95]

On that note, friendly reminder to see if we can re-run / enable the changed Azure pipeline on this PR somehow, so that it's ready for merging.

[MarijnS95]

@oddhack @rpavlik Where are we on this? I'd like to get this done to unblock #13 :)

[oddhack]

@oddhack @rpavlik Where are we on this? I'd like to get this done to unblock #13 :)

I think it's OK in principle? As long as it isn't auto-pushing the asciidoctor-spec / vulkan-* images and there's still a way to invoke the manual equivalent of the old build-and-push-one.sh, I'm good.

Regarding Azure I basically know nothing - years ago I had Azure CI configured on a Vulkan repo but have forgotten all the details, except that I had to go to a Microsoft website to see CI outputs. Probably the easiest way to re-run the Azure pipeline would be to force-push an empty commit?

[MarijnS95]

I think it's OK in principle? As long as it isn't auto-pushing the asciidoctor-spec / vulkan-* images and there's still a way to invoke the manual equivalent of the old build-and-push-one.sh, I'm good.

I haven't changed any of this yet. In a future PR where we change out Azure for GH actions it should be good to exclusively build-test all the images, but leave all publishing including openxr images to a manual action?

Please see https://github.com/KhronosGroup/DockerContainers/pull/15#discussion_r896129472 though, I changed the way this script is called. It'd be great if you can do a formal review of the code changes in this PR :)

Regarding Azure I basically know nothing - years ago I had Azure CI configured on a Vulkan repo but have forgotten all the details, except that I had to go to a Microsoft website to see CI outputs. Probably the easiest way to re-run the Azure pipeline would be to force-push an empty commit?

I've pushed to this branch on multiple occasions, and have a hunch changing the scripts requires "maintainer approval" before they run? That doesn't explain why #13 ran, where the scripts have also been altered. Will try again.

[MarijnS95]

https://dev.azure.com/khronosgroup/Shared/_build?definitionId=5 Looks like it doesn't run because I made a typo in the scripts! I had expected it to at least report that as a failed check here... All the more reasons to switch to GH CI soon :)

[MarijnS95]

https://github.com/KhronosGroup/DockerContainers/pull/15#issuecomment-1154431341 And I wrote the answer myself :)

[MarijnS95]

CI is green now!

[oddhack]

Looks good to me. BTW can I ask why all the force-pushes? If commits accumulate they'll just get squashed out when we merge the MR.

[MarijnS95]

@oddhack that's just the workflow I'm used to, and I don't like the noise that ends up in the final squash-merged commit (titles of all the "fixup" commits).

I guess it makes it harder for you to see what changed in every push, correct?

[oddhack]

@oddhack that's just the workflow I'm used to, and I don't like the noise that ends up in the final squash-merged commit (titles of all the "fixup" commits).

I guess it makes it harder for you to see what changed in every push, correct?

Right - something changed, but it's harder to tell what. Not a big deal.

[rpavlik]

interesting, because GitLab explicitly has a feature to let you compare there across force pushes. I'm surprised GitHub doesn't.

Looks fine to me if it's fine to @oddhack . The OpenXR image is pretty stable so I rarely even think about this repo.

[MarijnS95]

Github does have a "Changed since last viewed" button but it disappears as soon as you've "viewed", on its own terms (i.e. reload the page and its gone). All you have then is the link under the word force-pushed (blue here):

But it's worthless if I ended up rebasing on latest main at the same time, for example. I see where this is coming from.

If we're all done here I guess we can merge? Get the Rust image in soon after that, and do a GitHub conversion separately.

We didn't end up discussing the "repo" vs "tag" part of the name, but IMHO that deserves its own distinct documented/annotated PR regardless (just like a migration to GH Actions).