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

Pack version does not include meta-data in cbuild*.yml files #1339

Closed
jeromecoutant opened this issue Feb 27, 2024 · 17 comments · Fixed by #1382
Closed

Pack version does not include meta-data in cbuild*.yml files #1339

jeromecoutant opened this issue Feb 27, 2024 · 17 comments · Fixed by #1382
Assignees
Labels
bug Something isn't working discussion Indicates an issue being in discussion

Comments

@jeromecoutant
Copy link
Contributor

Hi

I got an issue during the convert procedure with packs with a version with metadata

csolution.yml file:
- pack: STMicroelectronics::mw_mbed_crypto

$ grep mw_mbed_crypto $CMSIS_PACK_ROOT/.Local/local_repository.pidx
<pdsc vendor="STMicroelectronics" name="mw_mbed_crypto" version="2.28.4+st.0.1.1" url="file:///C:/xxx/"/>

$ csolution convert -s xxx.csolution.yml -c xxx.GCC+B-U585I-IOT02A

generated cprj file has correct information:
<package name="mw_mbed_crypto" vendor="STMicroelectronics" version="2.28.4+st.0.1.1:2.28.4+st.0.1.1"/>

But generated cbuild-pack.yml is wrong:
- resolved-pack: STMicroelectronics::[email protected]

And generated cbuild.yml as well:
- pack: STMicroelectronics::[email protected]

  • [tool or library]
    csolution 2.2.1 for Windows
@jeromecoutant jeromecoutant added the bug Something isn't working label Feb 27, 2024
@jkrech jkrech changed the title Pack version is not correct in cbuild*yml files Pack version is not correct in cbuild*.yml files Mar 4, 2024
@jkrech jkrech moved this to Todo in CMSIS-Toolbox 2.3.0 Mar 5, 2024
@edriouk
Copy link
Collaborator

edriouk commented Mar 7, 2024

According to Semantic Versioning meta data should be ignored by any comparison making the following versions equal
6.0.0 == 6.0.0+meta1 == 6.0.0+meta2
Since Pack ID is constructed as Vendor::Name@Version, meta data should be stripped from the version.
The cprj file stores pack attributes taken from the pdsc, Pack ID gets constructed out of that later with meta-data removed.
In YAML files we always store already constructed Pack ID => it may not contain meta data.

It is possible to obtain meta-data from corresponding pack object by callingRteItem::GetVerison()method.
However, storing the meta data as a part of ID in YAML files may lead to forward compatibility issues, because not all tools correctly process it, e.g. [cpackget] must strip meta data from version

@edriouk edriouk changed the title Pack version is not correct in cbuild*.yml files Pack version dous not include meta-data in cbuild*.yml files Mar 7, 2024
@edriouk edriouk changed the title Pack version dous not include meta-data in cbuild*.yml files Pack version does not include meta-data in cbuild*.yml files Mar 7, 2024
@Torbjorn-Svensson
Copy link
Collaborator

Hi @edriouk,

According to https://semver.org, it's defining that build metadata should not be used for precedence, however, equality is equality and not the same as precedence.

So, from understanding is that we need to store also the metadata in the cbuild*.yml files to ensure that the same pack will be selected next time when using the cbuild-pack.yml file.

@edriouk
Copy link
Collaborator

edriouk commented Mar 7, 2024

@Torbjorn-Svensson , you cannot install a pack with exact meta-data even if you know it, and it is not possible to install two pack versions that only differ by meta data.
Meta-data could just provide you a hint of what exactly pack build was used at the time of the cbuild*.yml creation, but you cannot reproduce it in another environment.
For that purpose you should use revision (aka pre-release) information Major.Minor.Patch-Revision+meta

@Torbjorn-Svensson
Copy link
Collaborator

This seems to work when using cpackget add /tmp/SomeVendor.RteTest.0.0.1-alpha1+st.1.2.pack.
Are you referring to some other use-case?

This is the content of the pack root after adding the pack:

$ find $CMSIS_PACK_ROOT
/tmp/issue1339/
/tmp/issue1339/.Web
/tmp/issue1339/.Web/index.pidx
/tmp/issue1339/.Local
/tmp/issue1339/.Local/SomeVendor.RteTest.pdsc
/tmp/issue1339/.Local/local_repository.pidx
/tmp/issue1339/SomeVendor
/tmp/issue1339/SomeVendor/RteTest
/tmp/issue1339/SomeVendor/RteTest/0.0.1-alpha1+st.1.2
/tmp/issue1339/SomeVendor/RteTest/0.0.1-alpha1+st.1.2/PreInclude
/tmp/issue1339/SomeVendor/RteTest/0.0.1-alpha1+st.1.2/PreInclude/MyPreInclude.h
/tmp/issue1339/SomeVendor/RteTest/0.0.1-alpha1+st.1.2/PreInclude/MyLocalPreInclude.c
/tmp/issue1339/SomeVendor/RteTest/0.0.1-alpha1+st.1.2/PreInclude/MyLocalPreInclude.h
/tmp/issue1339/SomeVendor/RteTest/0.0.1-alpha1+st.1.2/SomeVendor.RteTest.pdsc
/tmp/issue1339/SomeVendor/RteTest/0.0.1-alpha1+st.2.3
/tmp/issue1339/SomeVendor/RteTest/0.0.1-alpha1+st.2.3/PreInclude
/tmp/issue1339/SomeVendor/RteTest/0.0.1-alpha1+st.2.3/PreInclude/MyPreInclude.h
/tmp/issue1339/SomeVendor/RteTest/0.0.1-alpha1+st.2.3/PreInclude/MyLocalPreInclude.c
/tmp/issue1339/SomeVendor/RteTest/0.0.1-alpha1+st.2.3/PreInclude/MyLocalPreInclude.h
/tmp/issue1339/SomeVendor/RteTest/0.0.1-alpha1+st.2.3/SomeVendor.RteTest.pdsc
/tmp/issue1339/.Download
/tmp/issue1339/.Download/SomeVendor.RteTest.0.0.1-alpha1+st.1.2.pack
/tmp/issue1339/.Download/SomeVendor.RteTest.0.0.1-alpha1+st.1.2.pdsc
/tmp/issue1339/.Download/SomeVendor.RteTest.0.0.1-alpha1+st.2.3.pack
/tmp/issue1339/.Download/SomeVendor.RteTest.0.0.1-alpha1+st.2.3.pdsc

The pack named SomeVendor.RteTest.0.0.1-alpha1+st.1.2 and SomeVendor.RteTest.0.0.1-alpha1+st.2.3 are just the pack SomeVendor.RteTest.0.0.1 in this repository that has been renamed.

As far as I can tell, cpackget does the right thing and treat them as 2 different entities.

@edriouk
Copy link
Collaborator

edriouk commented Mar 7, 2024

cpackget behavior is actually a bug. Our pack infrastructure does not allow to use meta-data. It should not appear neither in directory nor in file names because other tools ignore that.

@Torbjorn-Svensson
Copy link
Collaborator

Should we remove the support for the meta part from https://github.com/Open-CMSIS-Pack/devtools/blob/main/tools/projmgr/schemas/common.schema.json#L1071 and https://github.com/Open-CMSIS-Pack/Open-CMSIS-Pack-Spec/blob/main/schema/PACK.xsd#L1784 to help that it should not be used to identify the packs.

Seems we could close this issue as "invalid" in that case as there should never be 2 packs published that only differs in meta?

@edriouk
Copy link
Collaborator

edriouk commented Mar 8, 2024

Yes, we can close this issue. I believe we should remove meta part from YAML scheme, but leave it for pdsc one. The latter can be useful for the pack publisher.

@Torbjorn-Svensson
Copy link
Collaborator

@jkrech, @brondani: Do you agree to remove the metadata in the PackID type from the YAML schema?
If we do this, is it a 3.0.0 version?

@brondani
Copy link
Collaborator

brondani commented Mar 11, 2024

I think we could still accept the build metadata in the schema but strip it when parsing/handling pack versions.
A warning could be issued when the required and installed metadata do not match.

On the other hand removing it from the schema would make it clear to the project developer that build metadata is not handled, but strictly speaking changes that can potentially break existing projects are not backward compatible and therefore require a major version increment.

@Torbjorn-Svensson Torbjorn-Svensson removed their assignment Mar 14, 2024
@iomint
Copy link

iomint commented Mar 15, 2024

Following the discussion we had during this week Open-CMSIS-pack call, we aligned internally in ST and we concluded that we support Daniel's proposal:

I think we could still accept the build metadata in the schema but strip it when parsing/handling pack versions. A warning could be issued when the required and installed metadata do not match.

Side comment: Reinhard pointed to Open-CMSIS-Spec related to Semantic versioning usage, "Build metadata" is mentioned this way in the SemVer spec however it is possible -and ST does it - to use this possibility to use metadata to have other tag than ".build.xxx" see example in the first post of this thread :

pdsc vendor="STMicroelectronics" name="mw_mbed_crypto" version="2.28.4+st.0.1.1" url="file:///C:/xxx/"/

so I was wondering if in the Open-CMSIS-Pack spec, we could indicate just "metadata" rather than "Build metadata" to reflect this possibility

@ReinhardKeil
Copy link
Collaborator

I suggest that the CMSIS project uses the rules of Sematic Versioning 2.0. IMHO this means:

  • Build metadata is not used to identify a pack
  • As pre-release information should be only used during development, the pack version is recorded in *.cbuild-pack.ymlwithoutpre-release` information. This would make the final release with examples contained in a pack easier.
  • Currently cpackget expands the directory path with pre-release information (see below); IMHO this is not required (even when it does not harm).
C:\tmp\MyVendor>cpackget add MyVendor.MyPack.1.0.0-dev0.pack
I: Adding pack "MyVendor.MyPack.1.0.0-dev0.pack"
I: Extracting files to C:\Users\jkrech\AppData\Local\Arm\Packs\MyVendor\MyPack\1.0.0-dev0...

Public index files should not support pre-release information.

I believe there is not "golden" way, but we should align in the way how pre-release and build metadata is used. The current CMSIS-Toolbox user's guide is not explicit here: https://github.com/Open-CMSIS-Pack/cmsis-toolbox/blob/main/docs/YML-Input-Format.md#pack-name-conventions

@jkrech jkrech linked a pull request Mar 19, 2024 that will close this issue
jkrech pushed a commit that referenced this issue Mar 19, 2024
Strip build metadata when handling pack versions.
Throw a warning when specified metadata does not match the one from the
loaded pack.
Addresses
#1339 (comment)

Co-authored-by: Daniel Brondani <[email protected]>
@github-project-automation github-project-automation bot moved this from Todo to Done in CMSIS-Toolbox 2.3.0 Mar 19, 2024
@slhultgren
Copy link
Collaborator

I suggest that the CMSIS project uses the rules of Sematic Versioning 2.0. IMHO this means:

  • Build metadata is not used to identify a pack
  • As pre-release information should be only used during development, the pack version is recorded in *.cbuild-pack.ymlwithoutpre-release` information. This would make the final release with examples contained in a pack easier.
  • Currently cpackget expands the directory path with pre-release information (see below); IMHO this is not required (even when it does not harm).
C:\tmp\MyVendor>cpackget add MyVendor.MyPack.1.0.0-dev0.pack
I: Adding pack "MyVendor.MyPack.1.0.0-dev0.pack"
I: Extracting files to C:\Users\jkrech\AppData\Local\Arm\Packs\MyVendor\MyPack\1.0.0-dev0...

Public index files should not support pre-release information.

I believe there is not "golden" way, but we should align in the way how pre-release and build metadata is used. The current CMSIS-Toolbox user's guide is not explicit here: https://github.com/Open-CMSIS-Pack/cmsis-toolbox/blob/main/docs/YML-Input-Format.md#pack-name-conventions

I think we should be careful when talking about also ignoring the pre-release part.

First, back on build-metadata, Semver.org only talks about precedence, not identification. Which means that there is no way to reliably determine which build-metadata is the "newest". This makes sense since one use-case for build-metadata is to e.g. put a git-commit hash in it, which means sorting as string would give unreliable results. (IMO this still could leave the option to still use it for identification, but I can agree, since precedence is unclear, let's say we always ignore it even for identification)

For pre-release it is not the same situation. Semver.org clearly defines how to determine precedence.
I think we could very well have a situation were we want to release publicly alpha/beta pre-releases via pack index files, because this is a very convenient way to distribute it.

For this reason, we need to keep the same guarantees of stability that we have with the cbuild-pack.yml. It is fairly likely that alpha/beta packs are not exactly identical as final released ones.

@jkrech jkrech reopened this Mar 19, 2024
@jkrech
Copy link
Member

jkrech commented Mar 19, 2024

Sorry, got closed when merged a PR.
At this point we have only made changes to handling the build meta information.

@ReinhardKeil
Copy link
Collaborator

@slhultgren here is the scenario that I believe we should support:

pre-release information

  • A pack is developed that contains also examples. The examples refer to the this pack. As explained under reproducible builds the *.cbuild-pack.yml file is part of the example.
  • During development, pre-release information is used in the pack. The tools pickup the latest pre-release as described in Semver.org. In this step, a developer would need to change the *.cbuild-pack.yml everytime a new pre-release becomes available. This would make CI systems hard.
  • Once all tests are passed, the only change to get to a release pack is to remove pre-release information. If now pre-release information would be enforced by *.cbuild-pack.yml the project would not build anymore. The release process would involve that all examples are re-created with the release pack version, and the pack is created again to get the final pack. You could therefore not differ between a release version that requires updates to the example to become final and the release version that contains the final examples.

How do you want to overcome this complexity when pre-release is part of *.cbuild-pack.yml?

build metadata
The usage of build metadata may depend on the build system that you are using during development. This blog may be a good source: https://blogs.stonesteps.ca/1/p/80. I agree with the author that build metadata should not be used in the final release of the pack. Hence I would recommend to always ignore it in the CMSIS-Toolbox.

Let's agree on these topics.

@ReinhardKeil ReinhardKeil added the discussion Indicates an issue being in discussion label Mar 19, 2024
@jkrech jkrech reopened this Mar 19, 2024
@jkrech
Copy link
Member

jkrech commented Mar 20, 2024

Proposal: Add a new cbuild command for checking and preparing a solution for public release:

  cbuild release <solution-name>.csolution.yml

This command always processes all contexts of the specified CMSIS solution.
a) .cbuild-pack.yml includes the pack versions for all contexts and does not include pre-release pack versions
b) RTE config files are checked for all contexts - they must be up-to-date and a base@version file exists
c) cleans any other temporary directories / files generated by cmsis-toolbox

This command shall be executed prior to committing solutions for distribution via packs.

Simply removing the pre-release qualifier from *.cbuild-pack.yml will make "reproducible builds" hard during pack development.

@ReinhardKeil
Copy link
Collaborator

This somewhat conflicts with #1450 - item 2. I would like to come to a conclusion as currently *.cbuild-pack.yml records release qualifiers.

@ReinhardKeil
Copy link
Collaborator

@jeromecoutant the initial problem is fixed in CMSIS-Toolbox 2.3.0 - hence I'm closing this isssue. Feel free to reopen if you disagree.

#1484 now captures the proposal of a cbuild release command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working discussion Indicates an issue being in discussion
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

8 participants