nasa / fprime-tools

F´ Python tooling and helpers.
https://github.com/nasa/fprime
Apache License 2.0
20 stars 38 forks source link

Clean up passing flags to CMake #164

Closed sobkulir closed 12 months ago

sobkulir commented 1 year ago
F´ Version
Affected Component

Problem Description

Running fprime-tools generate should implement -Dxyz KEY=VAL to pass flags to CMake, however, this functionality doesn't work as expected :

$ fprime-tools generate --help
# ....
  -Dxyz DXYZ            Pass -D flags through to CMakes

Instead, it's implemented as -DKEY=VAL.

How to Reproduce

1) doens't work, but 2) does:

$ # 1) bad
$ fprime-tools generate -Dxyz KEY=VAL
$ # 2) good
$ fprime-tools generate -DKEY=VAL

Expected Behavior

Either 1) should work and 2) not, or 2) should be documented rather than 1).

Root cause

Relevant files: 1) https://github.com/sobkulir/fprime-tools/blob/devel/src/fprime/fbuild/cli.py#L212 2) https://github.com/sobkulir/fprime-tools/blob/devel/src/fprime/util/cli.py#L214

Extra

Also, the CMake flags are passed with syntax -D <var>[:<type>]=<value>, consider adding the parameter to the regex.

thomas-bc commented 1 year ago

@sobkulir

1) doens't work, but 2) does

The goal is to keep it consistent with the way CMake has implemented this, i.e. -DFOO=BAR. The way it's implemented in fprime-tools is a little hacky, but it seems to be working fine for me.

I guess the --help flag "-Dxyz DXYZ" can be a little misleading. I'll take a look at how this can be fixed (not as trivial as it sounds since we are purposefully not matching the arguments). Unless someone wants to take a crack at it?

sobkulir commented 1 year ago

I see, thanks for the quick answer! Reading my issue again, I think I overcomplicated it a bit, but you got it right -- I agree that the best way to go about it would be: 1) Making "-Dxyz DXYZ" in --help be "-D FOO=BAR" 2) Extending the regex to allow the extra space to be in line with the message in --help, i.e., r"-D\s?([a-zA-Z0-9_]+)=(.*)" 3) (optional) Allow, the type specifier in the regex, i.e., r"-D\s?([a-zA-Z0-9_]+(?::[A-Z]+)?)=(.*)" and change the help message to "-D FOO[:TYPE]=BAR"

thomas-bc commented 1 year ago

2)) was not possible due to argparse interpreting the space as specifying the platform argument. Implemented 1) and 3) in the above PR