Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow setting NANOPB_SRC_ROOT_FOLDER #117

Merged
merged 2 commits into from
Apr 1, 2024
Merged

Conversation

axel-h
Copy link
Member

@axel-h axel-h commented Mar 31, 2024

  • in CMake, INTERNAL has the same effect as FORCE, so passing any -DNANOPB_SRC_ROOT_FOLDER=... to CMake on the command line will be overwritten and there is no way to customize the path then.
  • the second commit removes the trailing slash of the variable, which seem more a cosmetic thing. Having a "//" on some paths in the variables that are created later seems not to cause any practical issues.

Axel Heider added 2 commits March 31, 2024 03:23
Copy link
Contributor

@Indanz Indanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why are you using a different NanoPB and why can't you put it in tools/nanopb instead? Or symlink tools/nanopb to wherever your nanopb is?

@axel-h
Copy link
Member Author

axel-h commented Mar 31, 2024

I am using the same nanoPB version, but just a different directory layout. Symlinking is not really an option, as this would be the only thing that needs the project workspace and tools folder then.

I did not get things to work with just adding nanoPB to the CMAKE_MODULE_PATH. Making this work could be another mire generic way, but it may need more changes.

@Indanz
Copy link
Contributor

Indanz commented Mar 31, 2024

The directory layout seems quite hard-coded, how do you manage the other tools paths? Is it worth trying to re-use settings.cmake? It doesn't seem to be doing that much useful work.

@axel-h
Copy link
Member Author

axel-h commented Mar 31, 2024

Providing -DCMAKE_MODULE_PATH=... works well enough for all other modules.

@axel-h
Copy link
Member Author

axel-h commented Mar 31, 2024

Concerning your question about using settings.cmake or not, that is where #40 is aiming for to allow full customization. But as stated there, settings.cmake contains some useful parts, so using it has some benefits.

@axel-h
Copy link
Member Author

axel-h commented Apr 1, 2024

Since sel4bech basically uses the same settings.cmake, but does not need the patch, I have been digging a bit more into this. It seems sel4testis running in to the issues described at https://stackoverflow.com/questions/75619926/does-cmake-set-internal-apply-force because CMake runs settings.cmake twice here (at least when targeting QEMU, the logs show this). The second runs finally makes INTERNAL act in FORCE-mode and overwrite the command line setting. In sel4bench it runs only once, so the command line value is used.
I still think we should not use INTERNAL here (and also apply this patch in sel4bench). There is a message we print in https://github.com/seL4/seL4_tools/blob/8c660fd6d8ba60aa4c76d5c9586e3892502d8e0e/cmake-tool/helpers/nanopb.cmake#L22 that also implies this values is supposed to be be something one can set on the command line.

@Indanz Indanz merged commit a8f83c5 into seL4:master Apr 1, 2024
20 checks passed
@axel-h axel-h deleted the patch-axel-2 branch April 1, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants