Additional target parameters by maikelvdh · Pull Request #47 · tomtom-international/cpp-dependencies

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

    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

    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

  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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

maikelvdh commented Oct 10, 2017 •

edited

Loading

maikelvdh Oct 23, 2017 •

edited

Loading

maikelvdh Oct 23, 2017 •

edited

Loading