Additional target parameters #47

maikelvdh · 2017-10-10T06:21:44Z

This will allow users freedom to specify additional target parameters in add_executable() and add_library() in more relaxed way, e.g:

add_library(MY_PROJECT_1 SHARED EXCLUDE_FROM_ALL my_source_1.cpp)
add_library(MY_PROJECT_2
  SHARED
  EXCLUDE_FROM_ALL
  my_source_2.cpp
)

Next it will no longer enforce the usage of STATIC libraries by default after regeneration, but leave the possibility for the user to leave it blank and obtain then then the default setting of CMake which is STATIC anyway. That behaviour can in turn then easily be adjusted by using the CMake flag BUILD_SHARED_LIBS.

dascandy · 2017-10-22T17:23:19Z

src/CmakeRegen.cpp

-        } else {
-            o << "\n";
-        }
+        comp.additionalTargetParameters.insert("INTERFACE");


This is definitely the wrong place to do this. This function really shouldn't change this argument.

Totally agree, was the easiest way to achieve this ;). I will lift it to the input side to have there header only detection.

dascandy · 2017-10-22T17:23:40Z

src/CmakeRegen.cpp

-        }
+        comp.additionalTargetParameters.insert("INTERFACE");
+    }
+    if (!comp.additionalTargetParameters.empty()) {


This if does nothing.

dascandy · 2017-10-22T17:25:10Z

src/Component.h

@@ -87,7 +87,7 @@ struct Component {
    bool hasAddonCmake;
    std::string type;
    size_t index, lowlink;
-    std::string additionalTargetParameters;
+    std::set<std::string> additionalTargetParameters;


Specific order is not relevant? This will result in EXCLUDE_FROM_ALL SHARED if you entered SHARED EXCLUDE_FROM_ALL, which may change the meaning. I'd use a vector here for that reason.

The order is not relevant here with regards of your example.

dascandy · 2017-10-22T17:25:42Z

src/Input.cpp

@@ -250,58 +250,60 @@ static void ReadCmakelist(const Configuration& config, std::unordered_map<std::s
    Component &comp = AddComponentDefinition(components, path.parent_path());
    bool inTargetDefinition = false;
    bool inAutoSection = false;
-    int parenLevel = 0;


This was paren, as in parentheses, not as in parent without a T. This new name is very wrong and confusing.

To be honest the parenLevel as in terms of the abbreviation was rather confusing me. Agree that parentLevel is also not great, but I would rather see a different name. Maybe we can give it a better name instead which isn't confusing for neither of us.

When checking again parenLevel is the best typed name will revert back to that.

dascandy · 2017-10-22T17:28:34Z

src/Input.cpp

+                                                           : line.length();
+            if (endOfLine > 0) {
+                const std::string targetLine(line.substr(0, endOfLine));
+                static const std::unordered_set<std::string> cmakeTargetParameters = { "ALIAS", "EXCLUDE_FROM_ALL", "GLOBAL", "IMPORTED", "INTERFACE", "MACOSX_BUNDLE", "MODULE", "OBJECT", "STATIC", "SHARED", "UNKNOWN", "WIN32" };


Smells like it should be configurable, or at least something you can add on to. Also, some of these change the meaning of other arguments, for example you don't list files after ALIAS. Is this actually tested to work well?

Not sure whether we want to have this set of options configurable it is highly depending on CMake itself. With regards of testing the inputs I don't think the cpp-dependencies should handle this at all, that is up to the CMake tool itself otherwise we will start to replicate all the logic here. If an user has already written an CMakeLists.txt we should expect that it is at least accepted by the CMake parser.

Actually when checking again we should make set of options which are actually allowed in combination and we should restrict ourselves to specific CMake versions as some options are new for example. So we should put a bit more thinking in how far we want to go here.

For example even the test scenario of INTERFACE EXCLUDE_FROM_ALL is not a valid definition.

dascandy · 2017-10-22T17:29:38Z

test/CmakeRegenTest.cpp

@@ -312,8 +312,7 @@ TEST(RegenerateCmakeAddTarget_Interface) {
  };

  const std::string expectedOutput(
-      "add_library(${PROJECT_NAME} INTERFACE\n"
-      "  EXCLUDE_FROM_ALL\n"
+      "add_library(${PROJECT_NAME} EXCLUDE_FROM_ALL INTERFACE\n"


Yeah... this is wrong. I want INTERFACE to come first as the defining type of the library.

As mentioned earlier there is no strict order necessary, so I could make it a vector instead, but then still it would be matching first EXCLUDE_FROM_ALL and later INTERFACE.

dascandy · 2017-10-22T17:30:18Z

test/InputTest.cpp

+  for (auto& pair : components) {
+    const Component& comp = *pair.second;
+    ASSERT(comp.additionalTargetParameters.size() == 2);
+    ASSERT(*comp.additionalTargetParameters.begin() == "EXCLUDE_FROM_ALL");


Why do you assert on the order?

Will change this to evaluate that both arguments are in the set present.

MaikelvandenHurk-TomTom added 2 commits October 9, 2017 15:08

Better tracking of additional CMake target parameters

6d02123

Don’t put target definitions to custom section

f4a8376

dascandy reviewed Oct 22, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Additional target parameters #47

Additional target parameters #47

Uh oh!

maikelvdh commented Oct 10, 2017 •

edited

Loading

Uh oh!

dascandy Oct 22, 2017

Uh oh!

maikelvdh Oct 23, 2017

Uh oh!

dascandy Oct 22, 2017

Uh oh!

dascandy Oct 22, 2017

Uh oh!

maikelvdh Oct 23, 2017

Uh oh!

dascandy Oct 22, 2017

Uh oh!

maikelvdh Oct 23, 2017 •

edited

Loading

Uh oh!

maikelvdh Oct 23, 2017

Uh oh!

dascandy Oct 22, 2017

Uh oh!

maikelvdh Oct 23, 2017

Uh oh!

maikelvdh Oct 23, 2017

Uh oh!

dascandy Oct 22, 2017

Uh oh!

maikelvdh Oct 23, 2017 •

edited

Loading

Uh oh!

dascandy Oct 22, 2017

Uh oh!

maikelvdh Oct 23, 2017

Uh oh!

Uh oh!

Additional target parameters #47

Are you sure you want to change the base?

Additional target parameters #47

Uh oh!

Conversation

maikelvdh commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maikelvdh Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maikelvdh Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maikelvdh commented Oct 10, 2017 •

edited

Loading

maikelvdh Oct 23, 2017 •

edited

Loading

maikelvdh Oct 23, 2017 •

edited

Loading