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

cmake: make DEPENDENCIES in protobuf_generate() a multi-value keyword #19175

Closed
wants to merge 1 commit into from

Conversation

71
Copy link
Contributor

@71 71 commented Nov 7, 2024

It was previously a one-value keyword, so given

protobuf_generate(
  ...
  DEPENDENCIES
    a
    b
)

a would be a dependency, and b would be discarded (or, more precisely, added to protobuf_generate_UNPARSED_ARGUMENTS).

This can be confirmed with the following changes:

diff --git a/cmake/protobuf-generate.cmake b/cmake/protobuf-generate.cmake
index 244407ee2..6dfbcef6d 100644
--- a/cmake/protobuf-generate.cmake
+++ b/cmake/protobuf-generate.cmake
@@ -10,6 +10,9 @@ function(protobuf_generate)

   cmake_parse_arguments(protobuf_generate "${_options}" "${_singleargs}" "${_multiargs}" "${ARGN}")

+  message("Deps: ${protobuf_generate_DEPENDENCIES}")
+  message("Unparsed args: ${protobuf_generate_UNPARSED_ARGUMENTS}")
+
   if(NOT protobuf_generate_PROTOS AND NOT protobuf_generate_TARGET)
     message(SEND_ERROR "Error: protobuf_generate called without any targets or source files")
     return()
diff --git a/cmake/tests.cmake b/cmake/tests.cmake
index 7976546fe..1cadf1e5a 100644
--- a/cmake/tests.cmake
+++ b/cmake/tests.cmake
@@ -27,6 +27,7 @@ foreach(proto_file ${lite_test_protos})
     LANGUAGE cpp
     OUT_VAR pb_generated_files
     IMPORT_DIRS ${protobuf_SOURCE_DIR}/src
+    DEPENDENCIES a b
   )
   set(lite_test_proto_files ${lite_test_proto_files} ${pb_generated_files})
 endforeach(proto_file)

Which printed "Deps: a" and "Unparsed args: b" for lite test files. With this change, it now prints "Deps: a;b" and "Unparsed args: ".

AFAIK, making DEPENDENCIES a multi-value keyword is mostly backwards-compatible; however, users who specified dependencies that were being ignored may be surprised to see that they are no longer ignored (which could cause build errors).

To prevent this kind of problem in the future, validating that protobuf_generate_UNPARSED_ARGUMENTS is empty could be useful, but to avoid potentially breaking more builds I did not do this in this PR.

It was previously a one-value keyword, so given

```cmake
protobuf_generate(
  ...
  DEPENDENCIES
    a
    b
)
```

`a` would be a dependency, and `b` would be discarded (or, more
precisely, added to `protobuf_generate_UNPARSED_ARGUMENTS`).

This can be confirmed with the following changes:

```cmake
diff --git a/cmake/protobuf-generate.cmake b/cmake/protobuf-generate.cmake
index 244407ee2..6dfbcef6d 100644
--- a/cmake/protobuf-generate.cmake
+++ b/cmake/protobuf-generate.cmake
@@ -10,6 +10,9 @@ function(protobuf_generate)

   cmake_parse_arguments(protobuf_generate "${_options}" "${_singleargs}" "${_multiargs}" "${ARGN}")

+  message("Deps: ${protobuf_generate_DEPENDENCIES}")
+  message("Unparsed args: ${protobuf_generate_UNPARSED_ARGUMENTS}")
+
   if(NOT protobuf_generate_PROTOS AND NOT protobuf_generate_TARGET)
     message(SEND_ERROR "Error: protobuf_generate called without any targets or source files")
     return()
diff --git a/cmake/tests.cmake b/cmake/tests.cmake
index 7976546fe..1cadf1e5a 100644
--- a/cmake/tests.cmake
+++ b/cmake/tests.cmake
@@ -27,6 +27,7 @@ foreach(proto_file ${lite_test_protos})
     LANGUAGE cpp
     OUT_VAR pb_generated_files
     IMPORT_DIRS ${protobuf_SOURCE_DIR}/src
+    DEPENDENCIES a b
   )
   set(lite_test_proto_files ${lite_test_proto_files} ${pb_generated_files})
 endforeach(proto_file)
```

Which printed "Deps: a" and "Unparsed args: b" for lite test files. With
this change, it now prints "Deps: a;b" and "Unparsed args: ".

AFAIK, making `DEPENDENCIES` a multi-value keyword is mostly
backwards-compatible; however, users who specified dependencies that
were being ignored may be surprised to see that they are no longer
ignored (which could cause build errors).

To prevent this kind of problem in the future, validating that
`protobuf_generate_UNPARSED_ARGUMENTS` is empty could be useful, but to
avoid potentially breaking more builds I did not do this in this PR.
@71 71 requested a review from a team as a code owner November 7, 2024 13:00
@71 71 requested review from haberman and removed request for a team November 7, 2024 13:00
@mkruskal-google mkruskal-google added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Nov 7, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Nov 7, 2024
@JasonLunn JasonLunn added the cmake label Nov 7, 2024
@mkruskal-google mkruskal-google added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Nov 7, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Nov 7, 2024
@mkruskal-google mkruskal-google self-requested a review November 7, 2024 18:29
@copybara-service copybara-service bot closed this in 52887e1 Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants