add removal of nodes / branches

[nwesem]

This PR tries to solve issue #194, by adding a delete attribute. So in the base specification or from overlays, we can now set delete: true on a branch or node which will delete them from the created tree.

Please refer to ./docs/vspec2x_arch.md to see an example. I added tests for all exporters but still would appreciate it if you gave this a quick spin to check if all necessary functionality has been implemented.

Thanks! Looking forward to your comments

[erikbosch]

There is a conflict, I would suggest that you do a squash and rebase. But apart from that it looks good.

[ppb2020]

Some of my comments lead to a broader discussion on licensing. Shall I open a discussion or issue to track it?

[ppb2020]

This example is possibly incorrect as it refers to a Row2 instance, which does not exist, per se, in the catalog. You may need a better example. Or, is it intentional that the deletion would support deleting specific instances? If so, I am not sure why the instance would be created in the first place...

As well, this raises a very interesting issue for which I will raise a separate issue or discussion point related to the MPL-2.0 license. Section 1.10b states that the license covers: "any new file in Source Code Form that contains any Covered Software". Given that one must provide in the overlay file the node path name of the node to be deleted, the license requires that this overlay file be made available upon request and that the license terms apply to this new file. The same applies to an overlay that modifies an existing signal (say, to change the unit or set a different number of instances).

This defeats the goals of providing the overlay mechanism to allow third-party implementors of a catalog to create their own version independent of the VSS version and license. We probably need a broader discussion of the license.

Indeed, the text at https://covesa.github.io/vehicle_signal_specification/license/ mentions that deletions are not considered to run afoul of the license but these statements are external to the license itself and, even if COVESA has no intention to enforce this part of the license, it remains that lawyers are generally worried by such statements. It's generally preferable to address this in the license itself. Anyway, a separate discussion to this PR, to be continued once I open a separate issue or discussion.

[nwesem]

This example is possibly incorrect as it refers to a Row2 instance, which does not exist, per se, in the catalog. You may need a better example. Or, is it intentional that the deletion would support deleting specific instances? If so, I am not sure why the instance would be created in the first place...

Up for discussion, to me it seemed like a good example to show that even deleting instances would work if delete is used from overlay, but maybe you are right and I should split it to show multiple examples:
I would add one example for deleting a node, one for deleting a branch and should we add one for the instances or how does deleting instances work currently? Maybe I'm missing some information here, could you explain? Let's say a car only has only one row instead of two, would this be removed from the base VehicleSignalSpecification.vspec?

[erikbosch]

@ppb2020 - concerning the license topic - there have been some previous discussions on license for VSS as some seem to prefer for example Apache over MPL due to concerns similar to yours. It is anyhow a separate discussion, check with @paulboyes if you want to discuss it further.

[ppb2020]

@ppb2020 - concerning the license topic - there have been some previous discussions on license for VSS as some seem to prefer for example Apache over MPL due to concerns similar to yours. It is anyhow a separate discussion, check with @paulboyes if you want to discuss it further.

Wilco.

[nwesem]

There is a conflict, I would suggest that you do a squash and rebase. But apart from that it looks good.

Hi @erikbosch, I'm having trouble to finish the rebase because of the obsolete and tests directories, is there a small chance that in this merge pre-commit wasn't run or that the upgrade of pre-commit is causing it? Same thing occurs when I run pre-commit hooks in the current master branch..

[erikbosch]

The Android part merged isn't conformant with pre-commit requirements (it is old code we do not want to touch). I can do a try and check how it works for me.

[erikbosch]

As there are merge-commits I think a rebase might trigger pre-commit checks, and they do not pass for the Android-related part. This could possibly be solved by first squashing. I tried this (upstream is official COVESA repo)

git fetch upstream pull/342/head:pr342
git fetch upstream
git checkout pr342
# Squash all by interactive rebase
git rebase -i 469bfa346b27e8
# Then rebase on latest master
# A few easily fixed conflicts
git rebase upstream/master
# After fixing them
git rebase --continue

I uploaded the result for reference and created a dummy-PR in my own fork to verify that build works

erikbosch#4

[nwesem]

thx @erikbosch, I tried it and it seems to be working fine. I found a bug though, currently deleting from overlay doesn't work as expected, will get back asap.

[nwesem]

Is there anyone that could help me solve this bug?

I added two more test "overlay" files that are supposed to remove a test node or branch from a base test.vspec but even though the overlay is loaded correctly and the delete element is detected as True, the delete element doesn't get overwritten by the overlay and therefore the node or branch is not deleted. Am I using it wrong? Do you have any ideas? I temporarily pushed the code here nwesem#5

[erikbosch]

@nwesem - I took a look and I think I found the problem

The problem seems to be that you delete the node from the overlay before merge, not from the final tree after merge. If you modify your function like this so that it actually does not remove the node you will see that the removal check will be called 3 times. Once before the merge and twice during the merge (I think)

    def check_for_removal(self):
        """Caution uses base node as input."""
        for node in LevelOrderIter(self):
            if node.delete:
                logging.info(f"Node {node.qualified_name()} will now be deleted - HAHA NOT!.")
                # node.parent = None
                # node.children = []

I think you should run the deletion after the merge. I did a test adding it to the function below and together with disabling check_for_removal it seems to give the right result, and the check is only performed once. You likely do not want to add the deletion to that function, at least not without renaming it, so some refactoring might be needed. If you experience other "strange merge errors" consider applying #346 to make sure it is not related to that error.

def verify_mandatory_attributes(node, abort_on_unknown_attribute: bool):
    """
    Verify that mandatory attributes are present.
    Need to be checked first after overlays (if any) have been applied, as attributes are not
    mandatory in individual files but only in the final tree
    """
    if isinstance(node, VSSNode):
        if node.delete:
            logging.info(f"Node {node.qualified_name()} will now be deleted - BUT NOW!.")
            node.parent = None
            node.children = []
        node.verify_attributes(abort_on_unknown_attribute)
        for child in node.children:
            verify_mandatory_attributes(child, abort_on_unknown_attribute)

[nwesem]

thx a lot @erikbosch, I have to get used to the codebase a bit more to solve these things, obviously it was deleted before the merge 😀

[nwesem]

created a squashed and rebased version of this PR here #348

therefore closing this PR