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

Fix make targets which generate mig-parted packages #41

Merged
merged 2 commits into from
Apr 18, 2024

Conversation

cdesiniotis
Copy link
Contributor

A previous commit, 6d18ec1, added a leading 'v' to our top level VERSION variable in versions.mk. Since then, building the mig-parted packages fails since the PACKAGE_VERSION (which now has a leading 'v') does not match the version string in our changelogs: https://github.com/NVIDIA/mig-parted/blob/v0.6.0/deployments/systemd/packages/Dockerfile.ubuntu#L53.

This PR strips the leading 'v' in the PACKAGE_VERSION and removes the ubuntu18.04 make targets since ubuntu18.04 tags are no longer being pushed for CUDA images starting with CUDA 12.3.2.

CUDA images are no longer pushing ubuntu18.04 tags starting with CUDA 12.3.2

Signed-off-by: Christopher Desiniotis <[email protected]>
@cdesiniotis cdesiniotis changed the title Fix pkg makefile Fix make targets which generate mig-parted packages Feb 29, 2024
Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

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

One note about removing the ubuntu images

@@ -45,24 +48,10 @@ tarball:
-v $$(pwd)/dist/$(@):/dist \
$(IMAGE):$(vVERSION)-$(@)

ubuntu18.04:
Copy link
Member

Choose a reason for hiding this comment

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

One concern I have here is that this will cause the packages we build to be incompatible with older debian-based systems. Note that these are not deployment images, but the images we use to build the binaries and package them.

This should be aligned with libnvidia-container or the nvidia-container-toolkit.

Copy link
Member

Choose a reason for hiding this comment

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

I have created #42 to do this. Note that we now have deb and rpm targets in this Makefile for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elezar thanks for working on the follow-up. Are there any pending items for this PR?

Copy link
Member

Choose a reason for hiding this comment

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

I think my concerns are addressed by #42.

@@ -25,17 +25,20 @@ REGISTRY ?= nvcr.io/nvidia
IMAGE := $(REGISTRY)/$(NAME)
endif

# strip 'v' from version string
PACKAGE_VERSION := $(VERSION:v%=%)
Copy link
Member

Choose a reason for hiding this comment

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

Out of scope for this PR: in the libnvidia-container and nvidia-container-toolkit packages, I started generating the changelog automatically as part of the build to streamline things. Not sure if that's something we want to try here.

@@ -45,24 +48,10 @@ tarball:
-v $$(pwd)/dist/$(@):/dist \
$(IMAGE):$(vVERSION)-$(@)
Copy link
Contributor

Choose a reason for hiding this comment

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

We seem to pre-pend the v to the VERSION var here. Does this not conflict with the following?

PACKAGE_VERSION := $(VERSION:v%=%)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vVERSION is a separate variable defined in the top-level versions.mk file and is used when we explicitly want a version string with the leading v. In this case, we are using it when constructing the image tag.

Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

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

Let's get this in for the consistent PACKAGE_VERSION change and then address any follow-ups in #42.

@elezar elezar merged commit 99631f4 into NVIDIA:main Apr 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants