search

Open-CMSIS-Pack / devtools

Open-CMSIS-Pack development tools - C++
Apache License 2.0
74 stars 57 forks source link

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

Closed jeromecoutant closed 6 months ago

jeromecoutant commented 8 months ago

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::mw_mbed_crypto@2.28.4

And generated cbuild.yml as well: - pack: STMicroelectronics::mw_mbed_crypto@2.28.4

edriouk commented 8 months ago

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

Torbjorn-Svensson commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 7 months ago

@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 commented 7 months ago

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.

iomint commented 7 months ago

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 commented 7 months ago

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

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

slhultgren commented 7 months ago

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 commented 7 months ago

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

ReinhardKeil commented 7 months ago

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

pre-release information

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.

jkrech commented 7 months ago

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 commented 6 months ago

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

ReinhardKeil commented 6 months ago

@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.