-
Notifications
You must be signed in to change notification settings - Fork 21
Support for a v9 build-env #35
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
base: master
Are you sure you want to change the base?
Conversation
XAPI build is not able to pass with a value of 1024. 2048 should still be low enough to leave container startup time reasonable. Signed-off-by: Yann Dirson <[email protected]>
Signed-off-by: Yann Dirson <[email protected]>
Signed-off-by: Yann Dirson <[email protected]>
There does not seem to be a reason for not passing that flag when already on amd64. Will make it simpler to handle multiple arch. Signed-off-by: Yann Dirson <[email protected]>
02bcee8
to
adce911
Compare
Many Alma 10 packages use the flat build directory structure, and so don't have the SPECS/SOURCES/etc. directories any more. Could you import detect spec file paths? |
Ah I had missed that one, looks like a good idea |
@@ -35,16 +35,19 @@ case "$1" in | |||
9.*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a comment on the commit description (not sure why github doesn't allow to put a comment on the description itself).
Originally-by
doesn't seem to be a standard git trailer; Co-authored-by
seems more appropriate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well that's more "started by one and continued by another", co-authoring to me rings more like pair-programming.
https://lore.kernel.org/git/[email protected]/ notes Based-on-patch-by
and Original-patch-by
and more used in the kernel. That's really free-text, and as I've been using that one a number of times already, I'm not sure changing now would bring much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Github recognized Co-authored-by
and properly displays the authors. I don't think it recognizes any other. Gitlab also recognizes Co-authored-by
, not sure for gitea. It's also mentioned in the git documentation, although at a trailer specific of the git project.
- https://git-scm.com/docs/SubmittingPatches
- https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors
- https://gitlab.com/gitlab-org/gitlab-foss/-/issues/31640
I don't think that's mandatory, but it's a good practice to acknowledge the authorship with the tools we have IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK
Since init-container runs following the dockerfile, there seems to be no reason to rerun it with every run.py invocation. Signed-off-by: Yann Dirson <[email protected]>
Will help simplifying init-container.sh Signed-off-by: Yann Dirson <[email protected]>
Will help simplifying init-container.sh Signed-off-by: Yann Dirson <[email protected]>
Even after dropping --rebuild-srpm, adding new rpmbuild options had to be done on 2 commandlines. At the same time, using a subshell to deal with change of working directory makes the new code easier. Signed-off-by: Yann Dirson <[email protected]>
Not the same as running the whole run.py under `time` control, especially when using `--no-exit`. Also, does not include the build-env setup (which includes installation of the build-deps of SRPMS). Quite trivial now that we use a subshell for the build itself. Signed-off-by: Yann Dirson <[email protected]>
Picked a slightly different logic for the flat-structure detection, separated support for source-fetching, and did the latter using Alma tools rather than the generic tool you used, which does not seem to properly support packages from Alma. |
I dropped the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worked with my shim-unsigned and grub2 packages using docker. Maybe @fallen can also give his opinion since he uses podman.
Originally-by: Tu Dinh <[email protected]> This assumes the distro has already been bootstrapped by building xcp-ng-release and will not be usable for this one, the relevant RUN clause has to be commented out for this. Signed-off-by: Yann Dirson <[email protected]>
That could not be done in a subshell. Also, don't attempt to download users repo when there is none, 404 HTML contents are a bad fit for yum.repos.d Signed-off-by: Yann Dirson <[email protected]>
Originally-by: Tu Dinh <[email protected]> Signed-off-by: Yann Dirson <[email protected]>
Necessary with podman < v5.5.1 to workaround a bug. Signed-off-by: Yann Dirson <[email protected]>
Signed-off-by: Yann Dirson <[email protected]>
Note that with podman < 5.5.1 we cannot have both linux/amd64 and linux/amd64/v2. And using a completely different arch requires emulation. Signed-off-by: Yann Dirson <[email protected]>
Signed-off-by: Yann Dirson <[email protected]>
This allows to build packages with a flat (current Fedora/RHEL standard) source layout. Originally-by: Tu Dinh <[email protected]> Signed-off-by: Yann Dirson <[email protected]>
…ux bucket Signed-off-by: Yann Dirson <[email protected]>
Notably useful for -bp, to - quickly get a patched source tree - apply patches using quilt, in order to refresh them in 8.3 env, so they will not be rejected by the 9.0 one Signed-off-by: Yann Dirson <[email protected]>
It is highly disrupting to have to add `set -x` manually to the script and rebuild it, each time something goes wrong. Signed-off-by: Yann Dirson <[email protected]>
Signed-off-by: Yann Dirson <[email protected]>
I gave this a try with both docker and podman. On podman I was able to generate the build image, but I was not able to build grub2 package:
|
@@ -15,10 +15,10 @@ | |||
import uuid | |||
|
|||
CONTAINER_PREFIX = "xcp-ng/xcp-ng-build-env" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CONTAINER_PREFIX = "xcp-ng/xcp-ng-build-env" | |
CONTAINER_PREFIX = "localhost/xcp-ng/xcp-ng-build-env" |
I had to do that to make podman not propose to interactively select a registry:
? Please select an image:
▸ registry.fedoraproject.org/xcp-ng/xcp-ng-build-env:9.0
registry.access.redhat.com/xcp-ng/xcp-ng-build-env:9.0
docker.io/xcp-ng/xcp-ng-build-env:9.0
it doesn't have that behavior without the --platform
option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be interesting to have the images pre-built in ghcr.io, and use ghcr.io in the image name, but I keep that for another PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to do that to make podman not propose to interactively select a registry:
I remember having to do that in one early test, but did not need to do that anymore recently 🤔
It may be interesting to have the images pre-built in ghcr.io, and use ghcr.io in the image name, but I keep that for another PR :)
IIRC @psafont mentionned he has plans for something like this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it would be good to upload the images so they are accessible to mock and avoid reinstalling packages to set the base :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to do that to make podman not propose to interactively select a registry:
I remember having to do that in one early test, but did not need to do that anymore recently 🤔
@glehmann actually when I add localhost/
, using podman 4.3.1, it fails in a very strange way, but that's apparently "just because" I omitted the --platform linux/amd64
workaround needed for that old version (podman < v5.5.1 has a bug where the container is not registed as linux/amd64/v2
).
The thing is, when the localhost/
prefix is not used the error is pretty easy to link with the bug when one knows about it:
Error: short-name "xcp-ng/xcp-ng-build-env:9.0" did not resolve to an alias and no unqualified-search registries are defined in "/etc/containers/registries.conf"
But with the prefix:
...
WARN[0002] Failed, retrying in 1s ... (3/3). Error: initializing source docker://localhost/xcp-ng/xcp-ng-build-env:9.0: pinging container registry localhost: Get "https://localhost/v2/": dial tcp [::1]:443: connect: connection refused
Error: initializing source docker://localhost/xcp-ng/xcp-ng-build-env:9.0: pinging container registry localhost: Get "https://localhost/v2/": dial tcp [::1]:443: connect: connection refused
We'll need to raise awareness of this behaviour, or some of us may get trapped :/
This is likely due to packages not in |
I used the same command line as for docker:
But it did not work. |
No description provided.