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

[dockers] change RPC, DBG dockers version: put RPG, DBG sign in build… #8920

Merged
merged 1 commit into from
Nov 1, 2021

Conversation

stepanblyschak
Copy link
Collaborator

… 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)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

… metadata part of the version

Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
@stepanblyschak
Copy link
Collaborator Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

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
Copy link
Collaborator

@qiluo-msft qiluo-msft Oct 14, 2021

Choose a reason for hiding this comment

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

1.0.0+rpc

What is the impact to the build result?
Is there a design of the version string?
If we keep -, seem it is just a parsing issue? #Closed

Copy link
Collaborator Author

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.

Copy link
Collaborator

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. 1.0.0-rpc is less then 1.0.0
  2. 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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

@qiluo-msft qiluo-msft Oct 19, 2021

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?

Copy link
Collaborator Author

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)
Copy link
Collaborator

@xumia xumia Oct 19, 2021

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

@qiluo-msft qiluo-msft left a 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.

@volodymyrsamotiy
Copy link
Collaborator

@xumia, could you please approve in case no additional comments?

@liat-grozovik
Copy link
Collaborator

Note: approved to be merged.

@liat-grozovik liat-grozovik merged commit 2ef97bb into sonic-net:master Nov 1, 2021
judyjoseph pushed a commit that referenced this pull request Nov 3, 2021
… 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>
xumia pushed a commit that referenced this pull request Nov 23, 2021
Why I did it
Fix a bug in sonic debug image build. That bug is imported in the following PR: #8920
liushilongbuaa added a commit that referenced this pull request Feb 7, 2022
Why I did it
Fix a bug in sonic debug image build. That bug is imported in the following PR: #8920
@stepanblyschak stepanblyschak deleted the rpc-dbg-fix-version branch September 23, 2022 13:33
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.

7 participants