You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "lordgamez (via GitHub)" <gi...@apache.org> on 2023/05/11 14:42:48 UTC
[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request, #1572: MINIFICPP-2027 Upgrade Google Cloud library to version 2.10.1
lordgamez opened a new pull request, #1572:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1572
There were some major breaking changes in the GCP library since our previously used 1.37.0 which failed our upload test with the GCS server version 1.45.1. Upgrading the library should solve this issue.
https://issues.apache.org/jira/browse/MINIFICPP-2027
---------------------
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on pull request #1572: MINIFICPP-2027 Upgrade Google Cloud library to version 2.10.1
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on PR #1572:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1572#issuecomment-1545285807
> The windows failure looks related:
>
> ```
>
> The following tests FAILED:
> 98 - DeleteGCSObjectTests.ServerGivesPermaError (Failed)
> 99 - DeleteGCSObjectTests.ServerGivesTransientErrors (Failed)
> 100 - DeleteGCSObjectTests.HandlingSuccessfullDeletion (Failed)
> 101 - DeleteGCSObjectTests.EmptyGeneration (Failed)
> 104 - FetchGCSObjectTests.ServerError (Failed)
> 105 - FetchGCSObjectTests.HappyPath (Failed)
> 106 - FetchGCSObjectTests.EmptyGeneration (Failed)
> 109 - GCPCredentialsTests.DefaultGCPCredentialsWithEnv (Failed)
> 117 - ListGCSBucketTests.ServerGivesPermaError (Failed)
> 118 - ListGCSBucketTests.ServerGivesTransientErrors (Failed)
> 119 - ListGCSBucketTests.WithoutVersions (Failed)
> 120 - ListGCSBucketTests.WithVersions (Failed)
> 122 - PutGCSObjectTests.BucketFromAttribute (Failed)
> 123 - PutGCSObjectTests.ServerGivesTransientErrors (Failed)
> 124 - PutGCSObjectTests.ServerGivesPermaError (Failed)
> Errors while running CTest
> 125 - PutGCSObjectTests.NonRequiredPropertiesAreMissing (Failed)
> 126 - PutGCSObjectTests.Crc32cMD5LocationTest (Failed)
> 127 - PutGCSObjectTests.DontOverwriteTest (Failed)
> 128 - PutGCSObjectTests.ValidServerSideEncryptionTest (Failed)
> 130 - PutGCSObjectTests.NoContentType (Failed)
> 131 - PutGCSObjectTests.ContentTypeFromAttribute (Failed)
> 132 - PutGCSObjectTests.ObjectACLTest (Failed)
> Error: Process completed with exit code 8.
> ```
Yes I moved the PR back to draft state due to this, I'm still investigating the issue
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1572: MINIFICPP-2027 Upgrade Google Cloud library to version 2.10.1
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1572:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1572#discussion_r1211872671
##########
extensions/gcp/tests/GCPCredentialsControllerServiceTests.cpp:
##########
@@ -89,7 +105,7 @@ TEST_F(GCPCredentialsTests, DefaultGCPCredentialsWithEnv) {
auto temp_directory = test_controller_.createTempDirectory();
auto path = create_mock_json_file(temp_directory);
ASSERT_TRUE(path.has_value());
- google::cloud::internal::SetEnv("GOOGLE_APPLICATION_CREDENTIALS", path->string());
+ setGoogleEnvironmentCredentials(path->string().c_str());
Review Comment:
That was my first thought, but unfortunately it does not work on Windows. In `setEnvironmentVariable` the `SetEnvironmentVariableA` is called on Windows which is different from the `_putenv_s`. For the changes to be visible for the Google library we need to use `_putenv_s`. The difference is explained here: https://github.com/googleapis/google-cloud-cpp/issues/100#issuecomment-357246853
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on pull request #1572: MINIFICPP-2027 Upgrade Google Cloud library to version 2.10.1
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on PR #1572:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1572#issuecomment-1559789851
Included the fix suggested in the following discussion: https://github.com/googleapis/google-cloud-cpp/discussions/11613 This solves the test issues on Windows
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1572: MINIFICPP-2027 Upgrade Google Cloud library to version 2.10.1
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1572:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1572#discussion_r1217643416
##########
extensions/gcp/tests/DeleteGCSObjectTests.cpp:
##########
@@ -86,9 +86,7 @@ TEST_F(DeleteGCSObjectTests, ServerGivesPermaError) {
}
TEST_F(DeleteGCSObjectTests, ServerGivesTransientErrors) {
- EXPECT_CALL(*delete_gcs_object_->mock_client_, DeleteObject)
- .WillOnce(testing::Return(TransientError()))
- .WillOnce(testing::Return(TransientError()));
Review Comment:
It is a change in the patch provided by the google cloud library as mentioned in this comment: https://github.com/googleapis/google-cloud-cpp/discussions/11613#discussioncomment-5973949
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1572: MINIFICPP-2027 Upgrade Google Cloud library to version 2.10.1
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1572:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1572#discussion_r1211872671
##########
extensions/gcp/tests/GCPCredentialsControllerServiceTests.cpp:
##########
@@ -89,7 +105,7 @@ TEST_F(GCPCredentialsTests, DefaultGCPCredentialsWithEnv) {
auto temp_directory = test_controller_.createTempDirectory();
auto path = create_mock_json_file(temp_directory);
ASSERT_TRUE(path.has_value());
- google::cloud::internal::SetEnv("GOOGLE_APPLICATION_CREDENTIALS", path->string());
+ setGoogleEnvironmentCredentials(path->string().c_str());
Review Comment:
That was my first thought, but unfortunately it does not work on Windows. In `setEnvironmentVariable` the `SetEnvironmentVariableA` which is different from the `_putenv_s`. For the changes to be visible for the Google library we need to use `_putenv_s`. The difference is explained here: https://github.com/googleapis/google-cloud-cpp/issues/100#issuecomment-357246853
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1572: MINIFICPP-2027 Upgrade Google Cloud library to version 2.10.1
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1572:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1572#discussion_r1217664870
##########
cmake/GoogleCloudCpp.cmake:
##########
@@ -22,17 +22,36 @@ include(Crc32c)
set(PATCH_FILE_1 "${CMAKE_SOURCE_DIR}/thirdparty/google-cloud-cpp/remove-find_package.patch")
set(PATCH_FILE_2 "${CMAKE_SOURCE_DIR}/thirdparty/google-cloud-cpp/nlohmann_lib_as_interface.patch")
-set(PC ${Bash_EXECUTABLE} -c "set -x &&\
- (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE_1}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE_1}\\\") &&\
- (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE_2}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE_2}\\\")")
+set(PATCH_FILE_3 "${CMAKE_SOURCE_DIR}/thirdparty/google-cloud-cpp/mock-client-without-decorators.patch")
+set(PATCH_FILE_4 "${CMAKE_SOURCE_DIR}/thirdparty/google-cloud-cpp/mock_client_target.patch")
+if (SKIP_TESTS)
+ set(PC ${Bash_EXECUTABLE} -c "set -x &&\
+ (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE_1}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE_1}\\\") &&\
+ (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE_2}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE_2}\\\")")
Review Comment:
I'm not sure about the reason, but the cmake configuration fails with the following when using only single quotes:
```
/usr/bin/patch" -p1 -R -s -f --dry-run -i "/home/ggyimesi/projects/nifi-minifi-cpp-fork/thirdparty/google-cloud-cpp/remove-find_package.patch" || "/usr/bin/patch" -p1 -N -i "/home/ggyimesi/projects/nifi-minifi-cpp-fork/thirdparty/google-cloud-cpp/remove-find_package.patch: -c: line 2: syntax error: unexpected end of file
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1572: MINIFICPP-2027 Upgrade Google Cloud library to version 2.10.1
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1572:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1572#discussion_r1211914535
##########
extensions/gcp/tests/GCPCredentialsControllerServiceTests.cpp:
##########
@@ -89,7 +105,7 @@ TEST_F(GCPCredentialsTests, DefaultGCPCredentialsWithEnv) {
auto temp_directory = test_controller_.createTempDirectory();
auto path = create_mock_json_file(temp_directory);
ASSERT_TRUE(path.has_value());
- google::cloud::internal::SetEnv("GOOGLE_APPLICATION_CREDENTIALS", path->string());
+ setGoogleEnvironmentCredentials(path->string().c_str());
Review Comment:
Now that you mention it, I remember something like this, maybe thats why I was using the internal function. Thanks for explaining :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1572: MINIFICPP-2027 Upgrade Google Cloud library to version 2.10.1
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1572:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1572#discussion_r1219100389
##########
cmake/GoogleCloudCpp.cmake:
##########
@@ -22,17 +22,36 @@ include(Crc32c)
set(PATCH_FILE_1 "${CMAKE_SOURCE_DIR}/thirdparty/google-cloud-cpp/remove-find_package.patch")
set(PATCH_FILE_2 "${CMAKE_SOURCE_DIR}/thirdparty/google-cloud-cpp/nlohmann_lib_as_interface.patch")
-set(PC ${Bash_EXECUTABLE} -c "set -x &&\
- (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE_1}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE_1}\\\") &&\
- (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE_2}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE_2}\\\")")
+set(PATCH_FILE_3 "${CMAKE_SOURCE_DIR}/thirdparty/google-cloud-cpp/mock-client-without-decorators.patch")
+set(PATCH_FILE_4 "${CMAKE_SOURCE_DIR}/thirdparty/google-cloud-cpp/mock_client_target.patch")
+if (SKIP_TESTS)
+ set(PC ${Bash_EXECUTABLE} -c "set -x &&\
+ (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE_1}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE_1}\\\") &&\
+ (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE_2}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE_2}\\\")")
Review Comment:
Sorry I meant to say single escapes, not single quotes. I used the same code you suggested and that resulted in the error I mentioned above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on pull request #1572: MINIFICPP-2027 Upgrade Google Cloud library to version 2.10.1
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on PR #1572:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1572#issuecomment-1544972389
The windows failure looks related:
```
The following tests FAILED:
98 - DeleteGCSObjectTests.ServerGivesPermaError (Failed)
99 - DeleteGCSObjectTests.ServerGivesTransientErrors (Failed)
100 - DeleteGCSObjectTests.HandlingSuccessfullDeletion (Failed)
101 - DeleteGCSObjectTests.EmptyGeneration (Failed)
104 - FetchGCSObjectTests.ServerError (Failed)
105 - FetchGCSObjectTests.HappyPath (Failed)
106 - FetchGCSObjectTests.EmptyGeneration (Failed)
109 - GCPCredentialsTests.DefaultGCPCredentialsWithEnv (Failed)
117 - ListGCSBucketTests.ServerGivesPermaError (Failed)
118 - ListGCSBucketTests.ServerGivesTransientErrors (Failed)
119 - ListGCSBucketTests.WithoutVersions (Failed)
120 - ListGCSBucketTests.WithVersions (Failed)
122 - PutGCSObjectTests.BucketFromAttribute (Failed)
123 - PutGCSObjectTests.ServerGivesTransientErrors (Failed)
124 - PutGCSObjectTests.ServerGivesPermaError (Failed)
Errors while running CTest
125 - PutGCSObjectTests.NonRequiredPropertiesAreMissing (Failed)
126 - PutGCSObjectTests.Crc32cMD5LocationTest (Failed)
127 - PutGCSObjectTests.DontOverwriteTest (Failed)
128 - PutGCSObjectTests.ValidServerSideEncryptionTest (Failed)
130 - PutGCSObjectTests.NoContentType (Failed)
131 - PutGCSObjectTests.ContentTypeFromAttribute (Failed)
132 - PutGCSObjectTests.ObjectACLTest (Failed)
Error: Process completed with exit code 8.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm closed pull request #1572: MINIFICPP-2027 Upgrade Google Cloud library to version 2.10.1
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm closed pull request #1572: MINIFICPP-2027 Upgrade Google Cloud library to version 2.10.1
URL: https://github.com/apache/nifi-minifi-cpp/pull/1572
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1572: MINIFICPP-2027 Upgrade Google Cloud library to version 2.10.1
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1572:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1572#discussion_r1211837353
##########
extensions/gcp/tests/GCPCredentialsControllerServiceTests.cpp:
##########
@@ -79,7 +95,7 @@ class GCPCredentialsTests : public ::testing::Test {
};
TEST_F(GCPCredentialsTests, DefaultGCPCredentialsWithoutEnv) {
- google::cloud::internal::UnsetEnv("GOOGLE_APPLICATION_CREDENTIALS");
+ unsetGoogleEnvironmentCredentials();
Review Comment:
```suggestion
org::apache::nifi::minifi::utils::Environment::unsetEnvironmentVariable("GOOGLE_APPLICATION_CREDENTIALS");
```
##########
extensions/gcp/tests/GCPCredentialsControllerServiceTests.cpp:
##########
@@ -89,7 +105,7 @@ TEST_F(GCPCredentialsTests, DefaultGCPCredentialsWithEnv) {
auto temp_directory = test_controller_.createTempDirectory();
auto path = create_mock_json_file(temp_directory);
ASSERT_TRUE(path.has_value());
- google::cloud::internal::SetEnv("GOOGLE_APPLICATION_CREDENTIALS", path->string());
+ setGoogleEnvironmentCredentials(path->string().c_str());
Review Comment:
```suggestion
org::apache::nifi::minifi::utils::Environment::setEnvironmentVariable("GOOGLE_APPLICATION_CREDENTIALS", path->c_str());
```
##########
extensions/gcp/tests/GCPCredentialsControllerServiceTests.cpp:
##########
@@ -63,6 +63,22 @@ std::optional<std::filesystem::path> create_mock_json_file(const std::filesystem
p.close();
return path;
}
+
+void setGoogleEnvironmentCredentials(char const* value) {
+#ifdef WIN32
+ _putenv_s("GOOGLE_APPLICATION_CREDENTIALS", value);
+#else
+ setenv("GOOGLE_APPLICATION_CREDENTIALS", value, 1);
+#endif
+}
+
+void unsetGoogleEnvironmentCredentials() {
+#ifdef _WIN32
+ (void)_putenv_s("GOOGLE_APPLICATION_CREDENTIALS", "");
+#else
+ unsetenv("GOOGLE_APPLICATION_CREDENTIALS");
+#endif
+}
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1572: MINIFICPP-2027 Upgrade Google Cloud library to version 2.10.1
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1572:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1572#discussion_r1218659254
##########
cmake/GoogleCloudCpp.cmake:
##########
@@ -22,17 +22,36 @@ include(Crc32c)
set(PATCH_FILE_1 "${CMAKE_SOURCE_DIR}/thirdparty/google-cloud-cpp/remove-find_package.patch")
set(PATCH_FILE_2 "${CMAKE_SOURCE_DIR}/thirdparty/google-cloud-cpp/nlohmann_lib_as_interface.patch")
-set(PC ${Bash_EXECUTABLE} -c "set -x &&\
- (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE_1}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE_1}\\\") &&\
- (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE_2}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE_2}\\\")")
+set(PATCH_FILE_3 "${CMAKE_SOURCE_DIR}/thirdparty/google-cloud-cpp/mock-client-without-decorators.patch")
+set(PATCH_FILE_4 "${CMAKE_SOURCE_DIR}/thirdparty/google-cloud-cpp/mock_client_target.patch")
+if (SKIP_TESTS)
+ set(PC ${Bash_EXECUTABLE} -c "set -x &&\
+ (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE_1}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE_1}\\\") &&\
+ (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE_2}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE_2}\\\")")
Review Comment:
I meant single escaping, not single quotes. Do they work? It's like that in most other places, although there are a few more double escaped instances as well.
```suggestion
(\"${Patch_EXECUTABLE}\" -p1 -R -s -f --dry-run -i \"${PATCH_FILE_1}\" || \"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE_1}\") &&\
(\"${Patch_EXECUTABLE}\" -p1 -R -s -f --dry-run -i \"${PATCH_FILE_2}\" || \"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE_2}\")")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1572: MINIFICPP-2027 Upgrade Google Cloud library to version 2.10.1
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1572:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1572#discussion_r1215453924
##########
cmake/GoogleCloudCpp.cmake:
##########
@@ -22,17 +22,36 @@ include(Crc32c)
set(PATCH_FILE_1 "${CMAKE_SOURCE_DIR}/thirdparty/google-cloud-cpp/remove-find_package.patch")
set(PATCH_FILE_2 "${CMAKE_SOURCE_DIR}/thirdparty/google-cloud-cpp/nlohmann_lib_as_interface.patch")
-set(PC ${Bash_EXECUTABLE} -c "set -x &&\
- (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE_1}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE_1}\\\") &&\
- (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE_2}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE_2}\\\")")
+set(PATCH_FILE_3 "${CMAKE_SOURCE_DIR}/thirdparty/google-cloud-cpp/mock-client-without-decorators.patch")
+set(PATCH_FILE_4 "${CMAKE_SOURCE_DIR}/thirdparty/google-cloud-cpp/mock_client_target.patch")
+if (SKIP_TESTS)
+ set(PC ${Bash_EXECUTABLE} -c "set -x &&\
+ (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE_1}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE_1}\\\") &&\
+ (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE_2}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE_2}\\\")")
Review Comment:
Why do we need double escaping for the backslashes? I'm guessing a single escaping for the CMake language should be enough, and the quotes are passed down to the shell, quoting the filenames properly.
##########
extensions/gcp/tests/DeleteGCSObjectTests.cpp:
##########
@@ -86,9 +86,7 @@ TEST_F(DeleteGCSObjectTests, ServerGivesPermaError) {
}
TEST_F(DeleteGCSObjectTests, ServerGivesTransientErrors) {
- EXPECT_CALL(*delete_gcs_object_->mock_client_, DeleteObject)
- .WillOnce(testing::Return(TransientError()))
- .WillOnce(testing::Return(TransientError()));
Review Comment:
Why was this duplicated before, and why can it work with a single `WillOnce` now?
##########
extensions/gcp/tests/GCPCredentialsControllerServiceTests.cpp:
##########
@@ -89,7 +105,7 @@ TEST_F(GCPCredentialsTests, DefaultGCPCredentialsWithEnv) {
auto temp_directory = test_controller_.createTempDirectory();
auto path = create_mock_json_file(temp_directory);
ASSERT_TRUE(path.has_value());
- google::cloud::internal::SetEnv("GOOGLE_APPLICATION_CREDENTIALS", path->string());
+ setGoogleEnvironmentCredentials(path->string().c_str());
Review Comment:
Could we adapt `setEnvironmentVariable` to apply to both kinds of environment variables on Windows instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1572: MINIFICPP-2027 Upgrade Google Cloud library to version 2.10.1
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1572:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1572#discussion_r1217667240
##########
extensions/gcp/tests/GCPCredentialsControllerServiceTests.cpp:
##########
@@ -89,7 +105,7 @@ TEST_F(GCPCredentialsTests, DefaultGCPCredentialsWithEnv) {
auto temp_directory = test_controller_.createTempDirectory();
auto path = create_mock_json_file(temp_directory);
ASSERT_TRUE(path.has_value());
- google::cloud::internal::SetEnv("GOOGLE_APPLICATION_CREDENTIALS", path->string());
+ setGoogleEnvironmentCredentials(path->string().c_str());
Review Comment:
I suppose that should be okay, added this change to 11f5ee48faf1d47b07c959be5d2392f35961d1fa
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org