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

Options subfolder #822

Merged
merged 2 commits into from Oct 13, 2022
Merged

Options subfolder #822

merged 2 commits into from Oct 13, 2022

Conversation

kurddt
Copy link
Contributor

@kurddt kurddt commented Oct 12, 2022

This fix the following use case where both .options and .proto are located in a folder hierarchy that need to be preserved for the generated files. Some of the proto files could be imported using their path related to the proto folder

proto
├── A
│ ├── A.proto
│ ├── A.options
├── B
│ ├── B.proto
│ ├── B.options
│ ├── A.proto
│ ├── A.options
app
├── CMakeLists.txt
├── build
│ ├── A
│ │ ├── A.pb.h
│ │ ├── A.pb.c
│ ├── B
│ │ ├── B.pb.h
│ │ ├── B.pb.c
│ │ ├── A.pb.h
│ │ ├── A.pb,c

The issues where:

  • It was quite hard to make sure that protoc takes the proto folder has project root and not one of the subdirectory
  • When the correct folder was taken as project root the plugin could not find options files (A/A.options B/B.options) because proto/A/ and proto/B/ where added to the option file but not proto/.

@PetteriAimonen
Copy link
Member

Is it possible that this could break builds for some of the existing users of the CMake rules?

The first proto_path supplied is used as the root of the project so
preferably use user provided path
Because the path of the protofile supplied to the plugin might
contains subfolders, the nanopb options directories should also include
the root of the project
@kurddt
Copy link
Contributor Author

kurddt commented Oct 12, 2022

The change about the discovery of the option files won't break anything.

The modification of the order of import path might be more problematic but I think it should be OK.
The following import path have been pushed back:

  • GENERATOR_PATH: This one does not contains any .proto, so not sure why it's needed by probably shouldn't be the project root
  • GENERATOR_CORE_DIR: This one contains nanopb.proto so it should not be considered as the project root
  • CMAKE_CURRENT_BINARY_DIR: This does not contains any .proto, o not sure why it's needed by probably shouldn't be the project root
    So no issue there.

When RELPATH is not set no logic is changed. And when set I don't see a correct use case where a user would like to have the generated NANOPB_GENERATE_CPP_APPEND_PATH before it.

@PetteriAimonen
Copy link
Member

Sounds good to me.

The pull request currently appears marked draft. Let me know when you consider it ready for merging.

@kurddt kurddt marked this pull request as ready for review October 13, 2022 11:45
@kurddt
Copy link
Contributor Author

kurddt commented Oct 13, 2022

I marked it ready for review, if you remember some recent contributor of the FindNanopb.cmake file it might be good to have their input on that

@PetteriAimonen PetteriAimonen merged commit ac91cb0 into nanopb:master Oct 13, 2022
@PetteriAimonen
Copy link
Member

Thanks!

I checked a few of the recent CMake contributors earlier, but they aren't particularly active on GitHub currently.
This seems straightforward enough that I just merged it directly.

The protoc include path order can be a complex things so I'll add a migration note just-in-case, even though I don't expect this to cause problems for anyone.

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.

None yet

2 participants