colcon / colcon-mixin

Extension for colcon to read CLI mixins from files
http://colcon.readthedocs.io
Apache License 2.0
2 stars 7 forks source link

Providing multiple mixins concatenates lists in reverse order #40

Open asasine opened 2 years ago

asasine commented 2 years ago

When using multiple mixins that each define cmake-args, the order they are concatenated is in the reverse order they are listed in the mixin list.

Given these mixins:

index.yaml

mixin:
  - a.mixin
  - b.mixin

a.mixin

{
    "build": {
        "a": {
            "cmake-args": [
                "-DFOO=A"
            ]
        }
    }
}

b.mixin

{
    "build": {
        "b": {
            "cmake-args": [
                "-DFOO=B"
            ]
        }
    }
}

and this command:

colcon build --mixin a b

the executed cmake command for every package looks like this:

Invoking command in '/path/to/package/build/package': /usr/bin/cmake /path/to/package/src -DFOO=B -DFOO=A ...

Since the order provided to cmake is -DFOO=B -DFOO=A, the value of FOO is A when in cmake's configure step. This is the reverse of the order provided to --mixin a b as I expected. Is this a bug or by design?

jeetee commented 1 year ago

From reading https://github.com/colcon/colcon.readthedocs.org/blame/main/reference/mixin-arguments.rst#L31-L32 this seems to be a bug to me; I've come across the same when trying to overwrite build locations using two mixins; Currently you have to swap the call order to

colcon build --mixin specific_override general_case

While the docs seem to suggest it should be the other way around.

cottsay commented 7 months ago

Hmm, this is a bit tricky. Simply reversing the application order causes list arguments to have the correct order, but messes up string arguments. The problem seems to center around how mixins PREPEND to list arguments.

We may need to pre-merge the mixin arguments before applying them to the final argument composition.

sdmcgrath commented 1 week ago

Hmm, this is a bit tricky. Simply reversing the application order causes list arguments to have the correct order, but messes up string arguments. The problem seems to center around how mixins PREPEND to list arguments.

@cottsay I tried to replicate @asasine's setup, but adding string arguments as well. e.g.

a.mixin

    "build": {
        "a": {
            "cmake-args": [
                "-DFOO=A"
            ],
            "cmake-target": "A"
        }
    }
}

b.mixin

    "build": {
        "b": {
            "cmake-args": [
                "-DFOO=B"
            ],
            "cmake-target": "B"
        }
    }
}

Executed colcon build --mixin a b and the output command had cmake_args=['-DFOO=B', '-DFOO=A'], cmake_target='A' Given that the documentation suggests later mixins will overwrite earlier ones, I would expect cmake_target='B', and so it looks to me like string arguments are already messed up (in addition to the list argument append order).