From de80240232b733bab0de9ee4cc05fdf19184a97e Mon Sep 17 00:00:00 2001 From: Martin Weber Date: Tue, 3 Nov 2020 21:58:54 +0100 Subject: [PATCH] Bug 567488: Use snakeyaml to persist command-line options to pass to cmake Change-Id: Ia6b60865f663aecae74d6d571bc9d213bf7cd36b Signed-off-by: Martin Weber --- .../META-INF/MANIFEST.MF | 3 +- .../CMakePropertiesControllerTest.java | 103 ++++++++++++++++++ .../CMakePropertiesEvolutionTest.java | 86 +++++++++++++++ .../META-INF/MANIFEST.MF | 3 +- .../internal/CMakeBuildConfiguration.java | 21 +++- .../internal/CMakePropertiesController.java | 57 ++++++++-- .../properties/CMakePropertiesBean.java | 3 +- .../core/properties/ICMakeProperties.java | 3 + .../ICMakePropertiesController.java | 13 ++- .../core/properties/IGeneralProperties.java | 2 + releng/org.eclipse.cdt.repo/category.xml | 1 + releng/org.eclipse.cdt.target/cdt.target | 3 +- 12 files changed, 274 insertions(+), 24 deletions(-) create mode 100644 cmake/org.eclipse.cdt.cmake.core.tests/src/org/eclipse/cdt/cmake/core/internal/CMakePropertiesControllerTest.java create mode 100644 cmake/org.eclipse.cdt.cmake.core.tests/src/org/eclipse/cdt/cmake/core/internal/CMakePropertiesEvolutionTest.java diff --git a/cmake/org.eclipse.cdt.cmake.core.tests/META-INF/MANIFEST.MF b/cmake/org.eclipse.cdt.cmake.core.tests/META-INF/MANIFEST.MF index 2d1777c237a..bc6a7e6aa49 100644 --- a/cmake/org.eclipse.cdt.cmake.core.tests/META-INF/MANIFEST.MF +++ b/cmake/org.eclipse.cdt.cmake.core.tests/META-INF/MANIFEST.MF @@ -8,5 +8,6 @@ Automatic-Module-Name: org.eclipse.cdt.cmake.core.tests Bundle-Vendor: %Bundle-Vendor Bundle-Copyright: %Bundle-Copyright Require-Bundle: org.junit, - org.junit.jupiter.api + org.junit.jupiter.api, + org.assertj;bundle-version="3.14.0" diff --git a/cmake/org.eclipse.cdt.cmake.core.tests/src/org/eclipse/cdt/cmake/core/internal/CMakePropertiesControllerTest.java b/cmake/org.eclipse.cdt.cmake.core.tests/src/org/eclipse/cdt/cmake/core/internal/CMakePropertiesControllerTest.java new file mode 100644 index 00000000000..66f61d86b0d --- /dev/null +++ b/cmake/org.eclipse.cdt.cmake.core.tests/src/org/eclipse/cdt/cmake/core/internal/CMakePropertiesControllerTest.java @@ -0,0 +1,103 @@ +/******************************************************************************* + * Copyright (c) 2020 Martin Weber. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ + +package org.eclipse.cdt.cmake.core.internal; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertNotNull; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.UUID; + +import org.eclipse.cdt.cmake.core.properties.CMakeGenerator; +import org.eclipse.cdt.cmake.core.properties.ICMakeProperties; +import org.eclipse.cdt.cmake.core.properties.IOsOverrides; +import org.junit.Test; + +/** + * @author Martin Weber + */ +public class CMakePropertiesControllerTest { + + /** + * Test method for {@link org.eclipse.cdt.cmake.core.internal.CMakePropertiesController#load()}. + * @throws IOException + */ + @Test + public void testLoad() throws IOException { + CMakePropertiesController testee; + // test with non-existing file + Path file = Path.of(new File("does-not-exist" + UUID.randomUUID().toString()).toURI()); + testee = new CMakePropertiesController(file, () -> { + }); + assertNotNull(testee.load()); + + // test with empty file + File f = File.createTempFile("CMakePropertiesControllerTest", null); + f.deleteOnExit(); + file = Path.of(f.toURI()); + testee = new CMakePropertiesController(file, () -> { + }); + assertNotNull(testee.load()); + } + + /** + * Test method for {@link org.eclipse.cdt.cmake.core.internal.CMakePropertiesController#save(org.eclipse.cdt.cmake.core.properties.ICMakeProperties)}. + * @throws IOException + */ + @Test + public void testSaveLoad() throws IOException { + Path file = Path.of(File.createTempFile("CMakePropertiesControllerTest", null).toURI()); + CMakePropertiesController testee = new CMakePropertiesController(file, () -> { + }); + ICMakeProperties props = testee.load(); + assertNotNull(props); + + props.setCacheFile("cacheFile"); + props.setClearCache(true); + props.setDebugOutput(true); + props.setDebugTryCompile(true); + props.setTrace(true); + props.setWarnNoDev(true); + props.setWarnUnitialized(true); + props.setWarnUnused(true); + { + IOsOverrides overrides = props.getLinuxOverrides(); + overrides.setGenerator(CMakeGenerator.Ninja); + List extraArgs = new ArrayList<>(); + extraArgs.add("arg1l=1"); + extraArgs.add("arg2l=2"); + overrides.setExtraArguments(extraArgs); + } + { + IOsOverrides overrides = props.getWindowsOverrides(); + overrides.setGenerator(CMakeGenerator.BorlandMakefiles); + List extraArgs = new ArrayList<>(); + extraArgs.add("arg1w=1"); + extraArgs.add("arg2w=2"); + overrides.setExtraArguments(extraArgs); + } + + List extraArgs = new ArrayList<>(); + extraArgs.add("arg1"); + extraArgs.add("arg2"); + props.setExtraArguments(extraArgs); + + testee.save(props); + + ICMakeProperties in = testee.load(); + assertThat(in).usingRecursiveComparison().isEqualTo(props); + } +} diff --git a/cmake/org.eclipse.cdt.cmake.core.tests/src/org/eclipse/cdt/cmake/core/internal/CMakePropertiesEvolutionTest.java b/cmake/org.eclipse.cdt.cmake.core.tests/src/org/eclipse/cdt/cmake/core/internal/CMakePropertiesEvolutionTest.java new file mode 100644 index 00000000000..3062a12f050 --- /dev/null +++ b/cmake/org.eclipse.cdt.cmake.core.tests/src/org/eclipse/cdt/cmake/core/internal/CMakePropertiesEvolutionTest.java @@ -0,0 +1,86 @@ +/******************************************************************************* + * Copyright (c) 2020 Martin Weber. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ + +package org.eclipse.cdt.cmake.core.internal; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; + +import org.eclipse.cdt.cmake.core.internal.properties.CMakePropertiesBean; +import org.eclipse.cdt.cmake.core.properties.CMakeGenerator; +import org.junit.Test; +import org.yaml.snakeyaml.Yaml; +import org.yaml.snakeyaml.constructor.CustomClassLoaderConstructor; + +/** + * @author Martin Weber + */ +public class CMakePropertiesEvolutionTest { + + private static final String VALUE_OF_EVOLVED_PROPERTY = "value of evolvedProperty"; + + /** Tests whether properties persisted by a previous version of our bundle can be loaded by + * a newer version of our bundle. + */ + @Test + public void testSaveLoadEvolution_1() throws IOException { + CMakePropertiesBean propsAtDefault = new CMakePropertiesBean(); + propsAtDefault.reset(true); + + CMakePropertiesBean props = new CMakePropertiesBean(); + props.setCacheFile("cacheFile"); + props.setClearCache(true); + props.setDebugOutput(true); + props.setDebugTryCompile(true); + props.setTrace(true); + props.setWarnNoDev(true); + props.setWarnUnitialized(true); + props.setWarnUnused(true); + props.getLinuxOverrides().setGenerator(CMakeGenerator.Ninja); + + List extraArgs = new ArrayList<>(); + extraArgs.add("arg1"); + extraArgs.add("arg2"); + props.setExtraArguments(extraArgs); + + Yaml yaml = new Yaml(new CustomClassLoaderConstructor(this.getClass().getClassLoader())); + String output = yaml.dump(props); + + // try to load as evolved properties.. + CMakePropertiesBean_1 in = yaml.loadAs(output, CMakePropertiesBean_1.class); + assertNotNull(in); + assertEquals(CMakePropertiesEvolutionTest.VALUE_OF_EVOLVED_PROPERTY, in.getEvolvedProperty()); + assertThat(props).usingRecursiveComparison().isEqualTo(in); + } + + private static class CMakePropertiesBean_1 extends CMakePropertiesBean { + private String evolvedProperty; + + @Override + public void reset(boolean resetOsOverrides) { + super.reset(resetOsOverrides); + evolvedProperty = CMakePropertiesEvolutionTest.VALUE_OF_EVOLVED_PROPERTY; + } + + public String getEvolvedProperty() { + return evolvedProperty; + } + + public void setEvolvedProperty(String evolvedProperty) { + this.evolvedProperty = evolvedProperty; + } + } +} diff --git a/cmake/org.eclipse.cdt.cmake.core/META-INF/MANIFEST.MF b/cmake/org.eclipse.cdt.cmake.core/META-INF/MANIFEST.MF index 7a0592dfa0e..078821a8d6d 100644 --- a/cmake/org.eclipse.cdt.cmake.core/META-INF/MANIFEST.MF +++ b/cmake/org.eclipse.cdt.cmake.core/META-INF/MANIFEST.MF @@ -12,7 +12,8 @@ Require-Bundle: org.eclipse.core.runtime, org.eclipse.cdt.core;bundle-version="5.12.0", org.eclipse.tools.templates.freemarker;bundle-version="1.0.0";visibility:=reexport, com.google.gson, - org.eclipse.cdt.cmake.is.core + org.eclipse.cdt.cmake.is.core, + org.yaml.snakeyaml;bundle-version="1.14.0" Bundle-RequiredExecutionEnvironment: JavaSE-11 Bundle-ActivationPolicy: lazy Export-Package: org.eclipse.cdt.cmake.core, diff --git a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/internal/CMakeBuildConfiguration.java b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/internal/CMakeBuildConfiguration.java index 40466251946..7c6a9e65b67 100644 --- a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/internal/CMakeBuildConfiguration.java +++ b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/internal/CMakeBuildConfiguration.java @@ -72,9 +72,8 @@ public class CMakeBuildConfiguration extends CBuildConfiguration { private ICMakeToolChainFile toolChainFile; - private final CMakePropertiesController pc = new CMakePropertiesController(() -> { - deleteCMakeCache = true; - }); + // lazily instantiated.. + private CMakePropertiesController pc; private Map infoPerResource; /** whether one of the CMakeLists.txt files in the project has been @@ -461,6 +460,20 @@ public class CMakeBuildConfiguration extends CBuildConfiguration { } } + /** Lazily creates the CMakePropertiesController for the project. + */ + private CMakePropertiesController getPropertiesController() { + if (pc == null) { + Path filePath = Path.of(getProject().getFile(".settings/CDT-cmake.yaml").getLocationURI()); //$NON-NLS-1$ + pc = new CMakePropertiesController(filePath, () -> { + deleteCMakeCache = true; + // TODO delete cache file here for the case a user restarts the workbench + // prior to running a new build + }); + } + return pc; + } + // interface IAdaptable @Override @SuppressWarnings("unchecked") @@ -468,7 +481,7 @@ public class CMakeBuildConfiguration extends CBuildConfiguration { T adapter0 = super.getAdapter(adapter); if (adapter0 == null) { if (ICMakePropertiesController.class.equals(adapter)) { - adapter0 = (T) pc; + adapter0 = (T) getPropertiesController(); } } return adapter0; diff --git a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/internal/CMakePropertiesController.java b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/internal/CMakePropertiesController.java index ed234f040e8..9eff0285f92 100644 --- a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/internal/CMakePropertiesController.java +++ b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/internal/CMakePropertiesController.java @@ -11,6 +11,13 @@ package org.eclipse.cdt.cmake.core.internal; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStreamWriter; +import java.io.Writer; +import java.nio.file.FileAlreadyExistsException; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.List; import java.util.Objects; import java.util.function.BinaryOperator; @@ -20,6 +27,8 @@ import org.eclipse.cdt.cmake.core.internal.properties.CMakePropertiesBean; import org.eclipse.cdt.cmake.core.properties.CMakeGenerator; import org.eclipse.cdt.cmake.core.properties.ICMakeProperties; import org.eclipse.cdt.cmake.core.properties.ICMakePropertiesController; +import org.yaml.snakeyaml.Yaml; +import org.yaml.snakeyaml.constructor.CustomClassLoaderConstructor; /** * A {@code ICMakePropertiesController} that monitors modifications to the project properties that force @@ -28,6 +37,7 @@ import org.eclipse.cdt.cmake.core.properties.ICMakePropertiesController; */ class CMakePropertiesController implements ICMakePropertiesController { + private final Path storageFile; private final Runnable cmakeCacheDirtyMarker; private String cacheFile; @@ -40,37 +50,60 @@ class CMakePropertiesController implements ICMakePropertiesController { /** Creates a new CMakePropertiesController object. * + * @param storageFile + * the file where to persist the properties. The file may not exist, but its parent directory is supposed to exist. * @param cmakeCacheDirtyMarker * the object to notify when modifications to the project properties force * us to delete file CMakeCache.txt to avoid complaints by cmake */ - CMakePropertiesController(Runnable cmakeCacheDirtyMarker) { + CMakePropertiesController(Path storageFile, Runnable cmakeCacheDirtyMarker) { + this.storageFile = Objects.requireNonNull(storageFile); this.cmakeCacheDirtyMarker = Objects.requireNonNull(cmakeCacheDirtyMarker); } @Override - public ICMakeProperties load() { - // TODO implement load() - CMakePropertiesBean props = new CMakePropertiesBean(); + public ICMakeProperties load() throws IOException { + CMakePropertiesBean props = null; + if (Files.exists(storageFile)) { + try (InputStream is = Files.newInputStream(storageFile)) { + props = new Yaml(new CustomClassLoaderConstructor(this.getClass().getClassLoader())).loadAs(is, + CMakePropertiesBean.class); + // props is null here if if no document was available in the file + } + } + if (props == null) { + // nothing was persisted, return default properties + props = new CMakePropertiesBean(); + } setupModifyDetection(props); return props; } @Override - public void save(ICMakeProperties properties) { + public void save(ICMakeProperties properties) throws IOException { // detect whether changes force us to delete file CMakeCache.txt to avoid complaints by cmake if (!Objects.equals(buildType, properties.getBuildType()) || !Objects.equals(cacheFile, properties.getCacheFile()) || !Objects.equals(generatorLinux, properties.getLinuxOverrides().getGenerator()) || !Objects.equals(generatorWindows, properties.getWindowsOverrides().getGenerator())) { cmakeCacheDirtyMarker.run(); // must remove cmake cachefile - } else if (extraArgumentsChange(extraArguments, properties.getExtraArguments()) - || extraArgumentsChange(extraArgumentsLinux, properties.getLinuxOverrides().getExtraArguments()) - || extraArgumentsChange(extraArgumentsWindows, properties.getWindowsOverrides().getExtraArguments())) { + } else if (hasExtraArgumentChanged(extraArguments, properties.getExtraArguments()) + || hasExtraArgumentChanged(extraArgumentsLinux, properties.getLinuxOverrides().getExtraArguments()) + || hasExtraArgumentChanged(extraArgumentsWindows, + properties.getWindowsOverrides().getExtraArguments())) { cmakeCacheDirtyMarker.run(); // must remove cmake cachefile } - // TODO implement save() + if (!Files.exists(storageFile)) { + try { + Files.createFile(storageFile); + } catch (FileAlreadyExistsException ignore) { + } + } + try (Writer wr = new OutputStreamWriter(Files.newOutputStream(storageFile))) { + new Yaml().dump(properties, wr); + } + setupModifyDetection(properties); } @@ -86,13 +119,13 @@ class CMakePropertiesController implements ICMakePropertiesController { extraArgumentsWindows = properties.getWindowsOverrides().getExtraArguments(); } - private boolean extraArgumentsChange(List args1, List args2) { + private boolean hasExtraArgumentChanged(List expected, List actual) { String wanted = "CMAKE_TOOLCHAIN_FILE"; //$NON-NLS-1$ // extract the last arguments that contain String wanted.. Predicate predContains = a -> a.contains(wanted); BinaryOperator keepLast = (first, second) -> second; - String a1 = args1.stream().filter(predContains).reduce(keepLast).orElse(null); - String a2 = args2.stream().filter(predContains).reduce(keepLast).orElse(null); + String a1 = expected.stream().filter(predContains).reduce(keepLast).orElse(null); + String a2 = actual.stream().filter(predContains).reduce(keepLast).orElse(null); if (!Objects.equals(a1, a2)) { return true; } diff --git a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/internal/properties/CMakePropertiesBean.java b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/internal/properties/CMakePropertiesBean.java index 241cc6beaf3..e24b40c8e66 100644 --- a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/internal/properties/CMakePropertiesBean.java +++ b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/internal/properties/CMakePropertiesBean.java @@ -13,7 +13,6 @@ package org.eclipse.cdt.cmake.core.internal.properties; import java.util.ArrayList; import java.util.List; -import java.util.Objects; import org.eclipse.cdt.cmake.core.properties.ICMakeProperties; @@ -144,7 +143,7 @@ public class CMakePropertiesBean implements ICMakeProperties { @Override public void setCacheFile(String cacheFile) { - this.cacheFile = Objects.requireNonNull(cacheFile); + this.cacheFile = cacheFile; } @Override diff --git a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/properties/ICMakeProperties.java b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/properties/ICMakeProperties.java index d81577bf27d..314698267f5 100644 --- a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/properties/ICMakeProperties.java +++ b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/properties/ICMakeProperties.java @@ -17,6 +17,9 @@ import java.util.List; * Holds project Properties for cmake. * * @author Martin Weber + * + * @noimplement This interface is not intended to be implemented by clients. + * @noextend This interface is not intended to be extended by clients. */ public interface ICMakeProperties { diff --git a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/properties/ICMakePropertiesController.java b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/properties/ICMakePropertiesController.java index 8549e96fd0f..ba0514ad8fd 100644 --- a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/properties/ICMakePropertiesController.java +++ b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/properties/ICMakePropertiesController.java @@ -11,18 +11,25 @@ package org.eclipse.cdt.cmake.core.properties; +import java.io.IOException; + /** - * Responsible for loading and persting @code ICMakeProperties} objects. + * Responsible for loading and persisting {@code ICMakeProperties} objects. * * @author Martin Weber */ public interface ICMakePropertiesController { /** Creates a new {@code ICMakeProperties} object, initialized from the persistence store. + * If the persistence store does not exist, an object initialized to the default values is returned. + * + * @throws IOException if the persistence store exists but could not be read */ - ICMakeProperties load(); + ICMakeProperties load() throws IOException; /** Saves the specified {@code ICMakeProperties} object to the persistence store. + * + * @throws IOException if the persistence store could not be written to */ - void save(ICMakeProperties properties); + void save(ICMakeProperties properties) throws IOException; } diff --git a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/properties/IGeneralProperties.java b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/properties/IGeneralProperties.java index baa34b0176e..11ceaac679b 100644 --- a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/properties/IGeneralProperties.java +++ b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/properties/IGeneralProperties.java @@ -16,6 +16,8 @@ package org.eclipse.cdt.cmake.core.properties; * * @author Martin Weber * + * @noimplement This interface is not intended to be implemented by clients. + * @noextend This interface is not intended to be extended by clients. */ public interface IGeneralProperties { diff --git a/releng/org.eclipse.cdt.repo/category.xml b/releng/org.eclipse.cdt.repo/category.xml index f6e6d7d291b..b6c8575ae69 100644 --- a/releng/org.eclipse.cdt.repo/category.xml +++ b/releng/org.eclipse.cdt.repo/category.xml @@ -208,6 +208,7 @@ + diff --git a/releng/org.eclipse.cdt.target/cdt.target b/releng/org.eclipse.cdt.target/cdt.target index 1eebce4a275..b3d5f27a175 100644 --- a/releng/org.eclipse.cdt.target/cdt.target +++ b/releng/org.eclipse.cdt.target/cdt.target @@ -72,6 +72,7 @@ + @@ -88,4 +89,4 @@ -ea -consolelog - + \ No newline at end of file