You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "bobpaulin (via GitHub)" <gi...@apache.org> on 2024/06/13 00:52:43 UTC
[PR] NIFI-13394: Check candidate virtual env for python command. [nifi]
bobpaulin opened a new pull request, #8961:
URL: https://github.com/apache/nifi/pull/8961
<!-- Licensed to the Apache Software Foundation (ASF) under one or more -->
<!-- contributor license agreements. See the NOTICE file distributed with -->
<!-- this work for additional information regarding copyright ownership. -->
<!-- The ASF licenses this file to You under the Apache License, Version 2.0 -->
<!-- (the "License"); you may not use this file except in compliance with -->
<!-- the License. You may obtain a copy of the License at -->
<!-- http://www.apache.org/licenses/LICENSE-2.0 -->
<!-- Unless required by applicable law or agreed to in writing, software -->
<!-- distributed under the License is distributed on an "AS IS" BASIS, -->
<!-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -->
<!-- See the License for the specific language governing permissions and -->
<!-- limitations under the License. -->
# Summary
[NIFI-13394](https://issues.apache.org/jira/browse/NIFI-13394)
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [ ] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [ ] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [ ] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [ ] Pull Request based on current revision of the `main` branch
- [ ] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
Built and started up on windows with python modules.
### Build
- [ ] Build completed using `mvn clean install -P contrib-check`
- [ ] JDK 21
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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
Re: [PR] NIFI-13394: Check candidate virtual env for python command. [nifi]
Posted by "bobpaulin (via GitHub)" <gi...@apache.org>.
bobpaulin commented on code in PR #8961:
URL: https://github.com/apache/nifi/pull/8961#discussion_r1641391058
##########
nifi-extension-bundles/nifi-py4j-bundle/nifi-py4j-bridge/src/main/java/org/apache/nifi/py4j/PythonProcess.java:
##########
@@ -302,8 +302,14 @@ String resolvePythonCommand() throws IOException {
} else if (virtualEnvDirectories.length == 1) {
commandExecutableDirectory = virtualEnvDirectories[0].getName();
} else {
- // Default to bin directory for macOS and Linux
- commandExecutableDirectory = "bin";
+ // We should get at most 2 directories. Check for python command.
+ if (virtualEnvDirectories[0].list((file, name) -> name.startsWith(pythonCmd)).length >= 1) {
Review Comment:
Yes I agree it is safer if I make it null safe and cleaner with a new method. The challenge was working with the virtualEnvDirectories array in a for loop and ensuring commandExecutableDirectory is only set exactly once. If I break I don't guarantee it's set if I loop without breaking it may be set multiple. So I've replaced this with a stream which is not the most efficient but does make the code readable and keeps the final guarantee or throws an exception.
--
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
Re: [PR] NIFI-13394: Check candidate virtual env for python command. [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory closed pull request #8961: NIFI-13394: Check candidate virtual env for python command.
URL: https://github.com/apache/nifi/pull/8961
--
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
Re: [PR] NIFI-13394: Check candidate virtual env for python command. [nifi]
Posted by "bobpaulin (via GitHub)" <gi...@apache.org>.
bobpaulin commented on code in PR #8961:
URL: https://github.com/apache/nifi/pull/8961#discussion_r1641391559
##########
nifi-extension-bundles/nifi-py4j-bundle/nifi-py4j-bridge/src/main/java/org/apache/nifi/py4j/PythonProcess.java:
##########
@@ -302,8 +302,14 @@ String resolvePythonCommand() throws IOException {
} else if (virtualEnvDirectories.length == 1) {
commandExecutableDirectory = virtualEnvDirectories[0].getName();
} else {
- // Default to bin directory for macOS and Linux
- commandExecutableDirectory = "bin";
+ // We should get at most 2 directories. Check for python command.
+ if (virtualEnvDirectories[0].list((file, name) -> name.startsWith(pythonCmd)).length >= 1) {
+ commandExecutableDirectory = virtualEnvDirectories[0].getName();
+ } else if (virtualEnvDirectories[1].list((file, name) -> name.startsWith(pythonCmd)).length >= 1) {
+ commandExecutableDirectory = virtualEnvDirectories[1].getName();
+ } else {
+ throw new IOException("Failed to find pythond command: " + pythonCmd);
Review Comment:
Thank you for catching that and for the additional feedback of course.
--
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
Re: [PR] NIFI-13394: Check candidate virtual env for python command. [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #8961:
URL: https://github.com/apache/nifi/pull/8961#discussion_r1640384193
##########
nifi-extension-bundles/nifi-py4j-bundle/nifi-py4j-bridge/src/main/java/org/apache/nifi/py4j/PythonProcess.java:
##########
@@ -302,8 +302,14 @@ String resolvePythonCommand() throws IOException {
} else if (virtualEnvDirectories.length == 1) {
commandExecutableDirectory = virtualEnvDirectories[0].getName();
} else {
- // Default to bin directory for macOS and Linux
- commandExecutableDirectory = "bin";
+ // We should get at most 2 directories. Check for python command.
+ if (virtualEnvDirectories[0].list((file, name) -> name.startsWith(pythonCmd)).length >= 1) {
Review Comment:
The `list()` method can return `null` under some circumstances, so adjusting the approach seems necessary. What about moving this block into a separate method named something like `findExecutableDirectory`? That method could iterate over the `virtualEnvDirectories` array, and not necessary assume that there will be at most two directories.
##########
nifi-extension-bundles/nifi-py4j-bundle/nifi-py4j-bridge/src/main/java/org/apache/nifi/py4j/PythonProcess.java:
##########
@@ -302,8 +302,14 @@ String resolvePythonCommand() throws IOException {
} else if (virtualEnvDirectories.length == 1) {
commandExecutableDirectory = virtualEnvDirectories[0].getName();
} else {
- // Default to bin directory for macOS and Linux
- commandExecutableDirectory = "bin";
+ // We should get at most 2 directories. Check for python command.
+ if (virtualEnvDirectories[0].list((file, name) -> name.startsWith(pythonCmd)).length >= 1) {
+ commandExecutableDirectory = virtualEnvDirectories[0].getName();
+ } else if (virtualEnvDirectories[1].list((file, name) -> name.startsWith(pythonCmd)).length >= 1) {
+ commandExecutableDirectory = virtualEnvDirectories[1].getName();
+ } else {
+ throw new IOException("Failed to find pythond command: " + pythonCmd);
Review Comment:
Minor spelling and string construction recommendation:
```suggestion
throw new IOException("Failed to find Python command [%s]".formatted(pythonCmd));
```
--
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