Spawn controller processes from a different directory on macOS #47013
Conversation
This is the Java side of elastic/ml-cpp#593 with a fallback so that ml-cpp bundles with either the new or old directory structure work for the time being. A few days after merging the C++ changes a followup to this change will be made that removes the fallback.
droberts195
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra |
|
Pinging @elastic/ml-core |
|
I've labelled this |
distribution/build.gradle
Outdated
| if (it.relativePath.segments[-2] == 'bin') { | ||
| // bin files, wherever they are within modules (eg platform specific) should be executable | ||
| it.mode = 0755 | ||
| } else if (platform == 'darwin' && it.relativePath.segments[-2] == 'MacOS') { |
There was a problem hiding this comment.
I wonder if this should be combined with previous if (||) so that we don't ever have to worry about maintenance on the previous block being forgotten in this block too? I can't think of a case where they'd deviate, but we can separate them if they need to be that way.
jasontedor
left a comment
jasontedor
left a comment
There was a problem hiding this comment.
Looks good, I left one suggestion, no need for another round though (I trust your judgement but would appreciate feedback one way or the other).
| final String parentDirName = file.getParent().getFileName().toString(); | ||
| if ("bin".equals(parentDirName)) { | ||
| setFileAttributes(file, BIN_FILES_PERMS); | ||
| } else if (Constants.MAC_OS_X && "MacOS".equals(parentDirName)) { // MacOS is an alternative to bin on macOS |
There was a problem hiding this comment.
Same comment here, I wonder if this would be better suited as an || with the if.
pugnascotia
left a comment
pugnascotia
left a comment
There was a problem hiding this comment.
I see a couple of comments about removing code before release. How will we ensure this happens?
I'll raise the PR to do it as soon as this one is merged. For some context look at the cross links to other issues in private repos in this PR. You will see that this is part of a major exercise. It won't get left half finished. I agree if the comments were more like "remove this in 8 months time" then they'd likely get forgotten and something to stop that would be needed. |
This change removes the temporary controller location fallback introduced in elastic#47013. Relates elastic/ml-cpp#593
|
I have merged this to master now to maximise the amount of time for it to propagate into local clones on developers' laptops. I will backport immediately after 7.4.0 is released. |
As @droberts195 mentions, in this case since its one step of work that he's actively working on, we would trust that he would continue with that work and wrap it up. For longer-running situations, we would add asserts to the codebase that would fail the build. For example assert that the current version is less than 9 if we wanted to make sure that something got removed from the codebase for version 9.0 of Elasticsearch. |
This is the Java side of elastic/ml-cpp#593 with a fallback so that ml-cpp bundles with either the new or old directory structure work for the time being. A few days after merging the C++ changes a followup to this change will be made that removes the fallback.
This is the Java side of elastic/ml-cpp#593 with a fallback so that ml-cpp bundles with either the new or old directory structure work for the time being. A few days after merging the C++ changes a followup to this change will be made that removes the fallback.
This change removes the temporary controller location fallback introduced in #47013. Relates elastic/ml-cpp#593
This change removes the temporary controller location fallback introduced in #47013. Relates elastic/ml-cpp#593
This change removes the temporary controller location fallback introduced in #47013. Relates elastic/ml-cpp#593
This is the Java side of elastic/ml-cpp#593 with a fallback so that ml-cpp bundles with either the new or old directory structure work for the time being. A few days after merging the C++ changes a followup to this change will be made that removes the fallback.
The tar.gz in 6.8 contains ML binaries for all platforms, so that check for the MacOS directory should not check the platform.
This change removes the temporary controller location fallback introduced in #47013. Relates elastic/ml-cpp#593
6 participants
This is the Java side of elastic/ml-cpp#593
with a fallback so that ml-cpp bundles with either the
new or old directory structure work for the time being.
A few days after merging the C++ changes a followup to
this change will be made that removes the fallback.