-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[dockers] change RPC, DBG dockers version: put RPG, DBG sign in build… #8920
[dockers] change RPC, DBG dockers version: put RPG, DBG sign in build… #8920
Conversation
… metadata part of the version Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
/azpw run |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -17,7 +17,7 @@ SONIC_INSTALL_DOCKER_IMAGES += $(DOCKER_SYNCD_BRCM_RPC) | |||
endif | |||
|
|||
$(DOCKER_SYNCD_BRCM_RPC)_CONTAINER_NAME = syncd | |||
$(DOCKER_SYNCD_BRCM_RPC)_VERSION = 1.0.0-rpc | |||
$(DOCKER_SYNCD_BRCM_RPC)_VERSION = 1.0.0+rpc |
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.
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.
Yes, there is a desing for version string, we are following https://semver.org/. This was a flaw of initial implementation. I chose to put rpc with a dash, which in semver means that this is a pre-release version, which it is not. It causes an extension installation failure that requires syncd>=1.0.0 but according to semver rules 1.0.0-rpc is less then 1.0.0. With this change I put rpc sign as a build metadata part which solves an issue because 1.0.0+rpc satisfies the constraint >=1.0.0.
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.
Trying to learn from you.
How semver is accepted by Debian?
You mentioned that
- 1.0.0-rpc is less then 1.0.0
- 1.0.0+rpc satisfies the constraint >=1.0.0
Are they just by chance, or they are defined by semver rules and implemented by Debian or apt system?
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.
Debian does not follow semver. They are defined by semver rules and there are online tools to check https://semvercompare.azurewebsites.net/?version=1.0.0&version=1.0.0-rpc&version=1.0.0%252Brpc.
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.
Why 1.0.0-rpc
< 1.0.0
<= 1.0.0+rpc
? Is it just string comparison?
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.
Here is a section defining this - https://semver.org/#spec-item-11.
@@ -840,7 +840,7 @@ $(addprefix $(TARGET_PATH)/, $(DOCKER_DBG_IMAGES)) : $(TARGET_PATH)/%-$(DBG_IMAG | |||
$(eval export $(subst -,_,$(notdir $($*.gz_PATH)))_dbg_pkgs=$(shell printf "$(subst $(SPACE),\n,$(call expand,$($*.gz_DBG_APT_PACKAGES),RDEPENDS))\n" | awk '!a[$$0]++')) | |||
./build_debug_docker_j2.sh $* $(subst -,_,$(notdir $($*.gz_PATH)))_dbg_debs $(subst -,_,$(notdir $($*.gz_PATH)))_image_dbgs > $($*.gz_PATH)/Dockerfile-dbg.j2 | |||
j2 $($*.gz_PATH)/Dockerfile-dbg.j2 > $($*.gz_PATH)/Dockerfile-dbg | |||
$(call generate_manifest,$*,-dbg) |
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.
Is it will change the docker image name or the package name? If no, please skip my comment.
There is a dependency on the "-dbg", see https://github.com/Azure/sonic-buildimage/blob/3bb248bd679d7e0b4b46490dbcc682577b60a4ed/scripts/versions_manager.py#L539
Is is possible to change to use "_dbg"? And change the file as well?
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.
Does not change docker image name nor package name.
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.
lgtm. Please wait @xumia approval.
@xumia, could you please approve in case no additional comments? |
Note: approved to be merged. |
… metadata part of the version (#8920) - Why I did it In case an app.ext requires a dependency syncd^1.0.0, the RPC version of syncd will not satisfy this constraint, since 1.0.0-rpc < 1.0.0. This is not correct to put 'rpc' as a prerelease identifier. Instead put 'rpc' as build metadata in the version: 1.0.0+rpc which satisfies the constraint ^1.0.0. - How I did it Changed the way how to version in RPC and DBG images are constructed. - How to verify it Install app.ext with syncd^1.0.0 dependency on a switch with RPC syncd docker. Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
Why I did it Fix a bug in sonic debug image build. That bug is imported in the following PR: #8920
Why I did it Fix a bug in sonic debug image build. That bug is imported in the following PR: #8920
… metadata part of the version
Signed-off-by: Stepan Blyshchak stepanb@nvidia.com
Why I did it
In case an app.ext requires a dependency syncd^1.0.0, the RPC version of syncd will not satisfy this constraint, since 1.0.0-rpc < 1.0.0. This is not correct to put 'rpc' as a prerelease identifier. Instead put 'rpc' as build metadata in the version: 1.0.0+rpc which satisfies the constraint ^1.0.0.
How I did it
Changed the way how to version in RPC and DBG images are constructed.
How to verify it
Install app.ext with syncd^1.0.0 dependency on a switch with RPC syncd docker.
Which release branch to backport (provide reason below if selected)
Description for the changelog
A picture of a cute animal (not mandatory but encouraged)