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

Spawn controller processes from a different directory on macOS #47013

Merged
merged 3 commits into from
Sep 25, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions distribution/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,9 @@ configure(subprojects.findAll { ['archives', 'packages'].contains(it.name) }) {
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') {
Copy link
Member

@jasontedor jasontedor Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// MacOS is an alternative to bin on macOS
it.mode = 0755
} else {
it.mode = 0644
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import joptsimple.OptionSpec;
import org.apache.lucene.search.spell.LevenshteinDistance;
import org.apache.lucene.util.CollectionUtil;
import org.apache.lucene.util.Constants;
import org.bouncycastle.bcpg.ArmoredInputStream;
import org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider;
import org.bouncycastle.openpgp.PGPException;
Expand Down Expand Up @@ -836,7 +837,10 @@ private void movePlugin(Path tmpRoot, Path destination) throws IOException {
Files.walkFileTree(destination, new SimpleFileVisitor<Path>() {
@Override
public FileVisitResult visitFile(final Path file, final BasicFileAttributes attrs) throws IOException {
if ("bin".equals(file.getParent().getFileName().toString())) {
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here, I wonder if this would be better suited as an || with the if.

setFileAttributes(file, BIN_FILES_PERMS);
} else {
setFileAttributes(file, PLUGIN_FILES_PERMS);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public class SpawnerNoBootstrapTests extends LuceneTestCase {
/**
* Simplest case: a module with no controller daemon.
*/
public void testNoControllerSpawn() throws IOException, InterruptedException {
public void testNoControllerSpawn() throws IOException {
Path esHome = createTempDir().resolve("esHome");
Settings.Builder settingsBuilder = Settings.builder();
settingsBuilder.put(Environment.PATH_HOME_SETTING.getKey(), esHome.toString());
Expand Down
8 changes: 6 additions & 2 deletions server/src/main/java/org/elasticsearch/bootstrap/Spawner.java
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,13 @@ void spawnNativeControllers(final Environment environment) throws IOException {
List<Path> paths = PluginsService.findPluginDirs(environment.modulesFile());
for (final Path modules : paths) {
final PluginInfo info = PluginInfo.readFromProperties(modules);
final Path spawnPath = Platforms.nativeControllerPath(modules);
Path spawnPath = Platforms.nativeControllerPath(modules);
if (!Files.isRegularFile(spawnPath)) {
continue;
// TODO: remove before release and just continue if the controller is not in the standard place
spawnPath = Platforms.fallbackNativeControllerPath(modules);
if (spawnPath == null || Files.isRegularFile(spawnPath) == false) {
continue;
}
}
if (!info.hasNativeController()) {
final String message = String.format(
Expand Down
30 changes: 29 additions & 1 deletion server/src/main/java/org/elasticsearch/plugins/Platforms.java
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@ private Platforms() {}
* The path to the native controller for a plugin with native components.
*/
public static Path nativeControllerPath(Path plugin) {
if (Constants.MAC_OS_X) {
return plugin
.resolve("platform")
.resolve(PLATFORM_NAME)
.resolve(PROGRAM_NAME + ".app")
.resolve("Contents")
.resolve("MacOS")
.resolve(PROGRAM_NAME);
}
return plugin
.resolve("platform")
.resolve(PLATFORM_NAME)
Expand All @@ -46,7 +55,26 @@ public static Path nativeControllerPath(Path plugin) {
}

/**
* Return the platform name based on the OS name and
* The fallback path to the native controller for a plugin with native
* components to be used if no program is found using the standard path.
* This is a temporary measure to allow developers not working on this
* functionality to continue to work with C++ bundles from before or
* after the change. This code should never be in a supported release.
* TODO: remove this method before release
*/
public static Path fallbackNativeControllerPath(Path plugin) {
if (Constants.MAC_OS_X) {
return plugin
.resolve("platform")
.resolve(PLATFORM_NAME)
.resolve("bin")
.resolve(PROGRAM_NAME);
}
return null;
}

/**
* Return the platform name based on the OS name and architecture, for example:
* - darwin-x86_64
* - linux-x86-64
* - windows-x86_64
Expand Down
47 changes: 47 additions & 0 deletions server/src/test/java/org/elasticsearch/plugins/PlatformsTests.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch 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.
*/
package org.elasticsearch.plugins;

import org.apache.lucene.util.Constants;
import org.elasticsearch.test.ESTestCase;

import java.nio.file.Path;

public class PlatformsTests extends ESTestCase {

public void testNativeControllerPath() {

final Path nativeControllerPath = Platforms.nativeControllerPath(createTempDir());

// The directory structure on macOS must match Apple's .app
// structure or Gatekeeper may refuse to run the program
if (Constants.MAC_OS_X) {
String programName = nativeControllerPath.getFileName().toString();
Path binDirectory = nativeControllerPath.getParent();
assertEquals("MacOS", binDirectory.getFileName().toString());
Path contentsDirectory = binDirectory.getParent();
assertEquals("Contents", contentsDirectory.getFileName().toString());
Path appDirectory = contentsDirectory.getParent();
assertEquals(programName + ".app", appDirectory.getFileName().toString());
} else {
Path binDirectory = nativeControllerPath.getParent();
assertEquals("bin", binDirectory.getFileName().toString());
}
}
}