add const UID attribute to allow constant IDs #352

nwesem · 2024-05-08T15:14:42Z

Hi this PR adds constUID attribute to allow overwriting generated static UIDs, as for static UID this is a 4-byte hex identifier. Please see test test_const_id for more details

Signed-off-by: Niclas Wesemann <[email protected]>

erikbosch · 2024-05-10T12:33:41Z

tests/vspec/test_static_uids/test_static_uids.py

+            + test_file
+            + " ./out.vspec --validate-static-uid "
+            + "./validation_vspecs/validation.vspec "
+            + "--only-validate-no-export"


To me it is not obvious why you have "--only-validate-no-export" here but not for the overlay alternative. Possibly add a comment.

Updated:

Now I noticed that in one of the overlay tests we actually check output. I suggest that we remove --only-validate-no-export here as well so that the two variants behave in the same way, i.e. both generate files. Or have a separate option to control that.

erikbosch · 2024-05-10T12:44:28Z

Should usage possibly be described in https://github.com/COVESA/vss-tools/blob/master/docs/vspec2id.md

erikbosch · 2024-05-13T06:38:54Z

vspec/model/vsstree.py

@@ -68,6 +68,7 @@ class VSSNode(Node):
        "$file_name$",
        "fka",
        "delete",
+        "constUID",


We should discuss how integrated we want the id-attributes to be in vss-tools. By adding constUID (and previously fka) they will be considered as core attributes and accepted in all tools.

There are two alternatives, one is to consider them as extended attributes and give warning unless they are specified by -e, the other is to refactor vss-tools so that each tool can add their own core attributes. Or alternatively that we consider them as "global core attributes", but then we should possibly mention them in https://github.com/COVESA/vss-tools/blob/master/docs/vspec2x_arch.md

@erikbosch can you please discuss this during the public meeting today? I have been sick yesterday / today, but can finish this tomorrow if you decide on how you want it to work. I would appreciate it, thank you!!

Lets keep this for now, just add some documentation, then I can do a quick look and if ok I merge

nwesem · 2024-05-14T13:45:04Z

Should usage possibly be described in https://github.com/COVESA/vss-tools/blob/master/docs/vspec2id.md

Thanks for the hint! I will add documentation as soon as we decided on how this will be integrated

erikbosch · 2024-05-14T14:21:13Z

MoM:

Erik and Niklas to sort out remaining steps, then ok to merge

nwesem · 2024-05-15T10:17:57Z

vspec/vssexporters/vss2id.py

+        assert node.constUID.startswith(
+            "0x"
+        ), f"constUID has to begin with '0x': {node.constUID}"
+        assert len(node.constUID) == 10, f"Invalid constUID: {node.constUID}"


I am considering to move these two assert statements to a utlils function and also run them on the generation of UIDs as an additional safety measure..

Found a better way to solve this here f46dd0c

Signed-off-by: Niclas Wesemann <[email protected]>

erikbosch · 2024-05-16T08:46:51Z

I assume there is still some work remaining, like documentation. Correct @nwesem?
If possible it would be great if you could finish during the day, as this is the last PR intended to be included in 4.2, so that I could prepare a release candidate tomorrow.

nwesem · 2024-05-16T09:00:19Z

thx for letting me know @erikbosch, I'm currently working on the documentation. Will be done soon. Would it be possible to include my suggestion for this issue #354 in the release candidate as well? I can do it for the shown min and max variables and send another PR by the end of the day. It's quite important to make static UID file iteration work.

Signed-off-by: Niclas Wesemann <[email protected]>

nwesem · 2024-05-16T13:31:06Z

@erikbosch quick question: to fix the bug mentioned in #354 I will branch off from here and merge back into this branch feat/const-id? Otherwise we would probably run into merge conflicts as I would be changing the same files.. do you agree that makes sense?

erikbosch · 2024-05-16T15:03:00Z

That works fine, no problem to use the same branch, or to use the "same" commit in multiple branches. Use what is easiest for you, as long as it is understandable what is the intended final result I have no problems fixing minor merge issues when a PR is to be merged.

nwesem · 2024-05-16T15:52:56Z

There is an issue. I implemented the default values for min and max and updated everything regarding the static UIDs but setting the default value causes 43 other tests to fail (AssertionErrors) because now min and max values are null or None instead of the empty string "".

@erikbosch Do you have a second to take a look and help me on this? I pushed it to branch https://github.com/nwesem/vss-tools/tree/fix/var-init
The other option is to find a somewhat hacky solution to make static UIDs work for 4.2 release and then properly solve it for 4.3 release. I gave a hacky solution a quick try, I could make it work but yeah that's definitely not good practice 😃
What do you think? It could also be worth spending the second to update the tests according to the new default values for min and max but I cannot estimate how much work that would be..

Also fyi @adobekan

Signed-off-by: Niclas Wesemann <[email protected]>

nwesem · 2024-05-17T07:30:11Z

Hi @erikbosch I found a decent way to temporarily fix it here ad0b093 which means this PR is ready for 4.2 release.
You can disregard the comment above for the moment. However, I will look into the initialization issue asap, but that needs more time. Let me know if this works for you.. Thx!!

erikbosch · 2024-05-17T08:08:11Z

Thanks @nwesem - that works for me. I will take a look at the PR and let you know if I have any concerns.

* add const UID attribute to allow constant IDs --------- Signed-off-by: Niclas Wesemann <[email protected]>

nwesem added 3 commits May 8, 2024 17:13

add const UID attribute to allow constant IDs

355e948

Signed-off-by: Niclas Wesemann <[email protected]>

fix pre-commit

2b4555a

Signed-off-by: Niclas Wesemann <[email protected]>

remove commented line in test

1bc798b

Signed-off-by: Niclas Wesemann <[email protected]>

erikbosch reviewed May 10, 2024

View reviewed changes

erikbosch reviewed May 13, 2024

View reviewed changes

nwesem commented May 15, 2024

View reviewed changes

nwesem added 3 commits May 15, 2024 13:44

make tests more complete by checking results and add clean up fixture

fc07c5c

Signed-off-by: Niclas Wesemann <[email protected]>

assert all UIDs generated and constant ones

f46dd0c

Signed-off-by: Niclas Wesemann <[email protected]>

fix mypy

621cb54

Signed-off-by: Niclas Wesemann <[email protected]>

nwesem added 3 commits May 16, 2024 14:52

add documentation for constant IDs

d3cf40f

Signed-off-by: Niclas Wesemann <[email protected]>

add one sentence regarding validation

21edf40

Signed-off-by: Niclas Wesemann <[email protected]>

auto format markdown

78d5784

Signed-off-by: Niclas Wesemann <[email protected]>

nwesem added 2 commits May 17, 2024 09:22

add test for iterated files

5d1b797

Signed-off-by: Niclas Wesemann <[email protected]>

export min/max using isinstance and update tests

ad0b093

Signed-off-by: Niclas Wesemann <[email protected]>

erikbosch merged commit 363a0a6 into COVESA:master May 17, 2024
5 checks passed

erikbosch pushed a commit that referenced this pull request May 17, 2024

add const UID attribute to allow constant IDs (#352)

a30bb0a

* add const UID attribute to allow constant IDs --------- Signed-off-by: Niclas Wesemann <[email protected]>

nwesem deleted the feat/const-id branch July 3, 2024 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add const UID attribute to allow constant IDs #352

add const UID attribute to allow constant IDs #352

nwesem commented May 8, 2024 •

edited

Loading

erikbosch May 10, 2024 •

edited

Loading

erikbosch commented May 10, 2024

erikbosch May 13, 2024

nwesem May 14, 2024 •

edited

Loading

erikbosch May 14, 2024

nwesem commented May 14, 2024

erikbosch commented May 14, 2024

nwesem May 15, 2024

nwesem May 15, 2024

erikbosch commented May 16, 2024

nwesem commented May 16, 2024

nwesem commented May 16, 2024

erikbosch commented May 16, 2024

nwesem commented May 16, 2024

nwesem commented May 17, 2024

erikbosch commented May 17, 2024

add const UID attribute to allow constant IDs #352

add const UID attribute to allow constant IDs #352

Conversation

nwesem commented May 8, 2024 • edited Loading

erikbosch May 10, 2024 • edited Loading

Choose a reason for hiding this comment

erikbosch commented May 10, 2024

erikbosch May 13, 2024

Choose a reason for hiding this comment

nwesem May 14, 2024 • edited Loading

Choose a reason for hiding this comment

erikbosch May 14, 2024

Choose a reason for hiding this comment

nwesem commented May 14, 2024

erikbosch commented May 14, 2024

nwesem May 15, 2024

Choose a reason for hiding this comment

nwesem May 15, 2024

Choose a reason for hiding this comment

erikbosch commented May 16, 2024

nwesem commented May 16, 2024

nwesem commented May 16, 2024

erikbosch commented May 16, 2024

nwesem commented May 16, 2024

nwesem commented May 17, 2024

erikbosch commented May 17, 2024

nwesem commented May 8, 2024 •

edited

Loading

erikbosch May 10, 2024 •

edited

Loading

nwesem May 14, 2024 •

edited

Loading