Skip to content

Additional target parameters #47

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.

Sign up for GitHub

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

Open
maikelvdh wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Additional target parameters #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d02123
Better tracking of additional CMake target parameters
MaikelvandenHurk-TomTom Oct 9, 2017
f4a8376
Don’t put target definitions to custom section
MaikelvandenHurk-TomTom Oct 9, 2017
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
23 changes: 12 additions & 11 deletions src/CmakeRegen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ void RegenerateCmakeFilesForComponent(const Configuration& config, Component *co
RegenerateCmakeHeader(o, config);

if (comp->root == ".") {
// Do this for the user, so that you don't get a warning on either
// Do this for the user, so that you don't get a warning on either
// not doing this, or doing this in the addon file.
o << "cmake_minimum_required(VERSION " << config.cmakeVersion << ")\n";
}
Expand Down Expand Up @@ -186,30 +186,31 @@ void RegenerateCmakeAddSubdirectory(std::ostream& o,
for (auto subdir : subdirs) {
o << "add_subdirectory(" << subdir << ")\n";
}

}

void RegenerateCmakeAddTarget(std::ostream& o,
const Configuration& config,
const Component& comp,
Component& comp,
const std::list<std::string>& files,
bool isHeaderOnly) {
o << comp.type << "(${PROJECT_NAME}";

if (isHeaderOnly) {
o << " INTERFACE\n";
} else {
if (config.addLibraryAliases.count(comp.type) == 1) {
o << " STATIC\n";
} else {
o << "\n";
}
comp.additionalTargetParameters.insert("INTERFACE");
Copy link
Collaborator

@dascandy dascandy Oct 22, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@maikelvdh maikelvdh Oct 23, 2017

Choose a reason for hiding this comment

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

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

}
if (!comp.additionalTargetParameters.empty()) {
Copy link
Collaborator

@dascandy dascandy Oct 22, 2017

Choose a reason for hiding this comment

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

This if does nothing.

for (auto &s : comp.additionalTargetParameters) {
o << " " << s;
}
}
o << "\n";

if (!isHeaderOnly) {
for (auto &f : files) {
o << " " << f << "\n";
}
}
o << comp.additionalTargetParameters;
o << ")\n\n";
}

Expand Down
4 changes: 2 additions & 2 deletions src/CmakeRegen.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ struct Configuration;

void RegenerateCmakeFilesForComponent(const Configuration& config,
Component *comp,
bool dryRun,
bool dryRun,
bool writeToStdout);

void MakeCmakeComment(std::string& cmakeComment,
Expand All @@ -39,7 +39,7 @@ void RegenerateCmakeAddSubdirectory(std::ostream& o,
const Component& comp);
void RegenerateCmakeAddTarget(std::ostream& o,
const Configuration& config,
const Component& comp,
Component& comp,
const std::list<std::string>& files,
bool isHeaderOnly);
void RegenerateCmakeHeader(std::ostream& o, const Configuration& config);
Expand Down
4 changes: 1 addition & 3 deletions src/Component.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ struct Component {
bool hasAddonCmake;
std::string type;
size_t index, lowlink;
std::string additionalTargetParameters;
std::set<std::string> additionalTargetParameters;
Copy link
Collaborator

@dascandy dascandy Oct 22, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@maikelvdh maikelvdh Oct 23, 2017

Choose a reason for hiding this comment

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

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

std::string additionalCmakeDeclarations;
bool onStack;
};
Expand All @@ -106,5 +106,3 @@ void CreateIncludeLookupTable(std::unordered_map<std::string, File>& files,
std::map<std::string, std::set<std::string>> &collisions);

#endif


54 changes: 27 additions & 27 deletions src/Input.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ static bool IsItemBlacklisted(const Configuration& config, const filesystem::pat
if (pathS.compare(2, s.size(), s) == 0) {
return true;
}
if (s == fileName)
if (s == fileName)
return true;
}
return false;
Expand Down Expand Up @@ -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;
Copy link
Collaborator

@dascandy dascandy Oct 22, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@maikelvdh maikelvdh Oct 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@maikelvdh maikelvdh Oct 23, 2017

Choose a reason for hiding this comment

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

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

int parentLevel = 0;
do {
getline(in, line);
if (strstr(line.c_str(), config.regenTag.c_str())) {
comp.recreate = true;
continue;
}
if (line.size() > 0 && line[0] == '#') {
if ((line.size() > 0 && line[0] == '#') || line.empty()) {
continue;
}
int newParenLevel = parenLevel + std::count(line.begin(), line.end(), '(')
int newParentLevel = parentLevel + std::count(line.begin(), line.end(), '(')
- std::count(line.begin(), line.end(), ')');
if (strstr(line.c_str(), "project(") == line.c_str()) {
size_t end = line.find(')');
if (end != line.npos) {
comp.name = line.substr(8, end - 8);
}
}
else if (TryGetComponentType(comp, config.addLibraryAliases, line)) {
} else if (TryGetComponentType(comp, config.addLibraryAliases, line)) {
inTargetDefinition = true;
} else if (TryGetComponentType(comp, config.addExecutableAliases, line)) {
inTargetDefinition = true;
} else if (inTargetDefinition) {
const size_t endOfLine = (newParenLevel == 0) ? line.find_last_of(')')
: line.length();
if (endOfLine > 0) {
const std::string targetLine(line.substr(0, endOfLine));
if (!strstr(targetLine.c_str(), "${IMPLEMENTATION_SOURCES}") &&
!strstr(targetLine.c_str(), "${IMPLEMENTATION_HEADERS}") &&
!IsCode(filesystem::path(targetLine).extension().generic_string())) {
comp.additionalTargetParameters.append(targetLine + '\n');
}
}
if (newParenLevel == 0) {
inTargetDefinition = false;
}
} else if (config.reuseCustomSections) {
if (IsAutomaticlyGeneratedSection(line))
{
inAutoSection = true;
} else {
if (!inAutoSection && !line.empty()) {
if (!inAutoSection && !line.empty() && !inTargetDefinition) {
comp.additionalCmakeDeclarations.append(line + '\n');
}
}
if (inAutoSection && newParenLevel == 0) {
if (inAutoSection && newParentLevel == 0) {
inAutoSection = false;
}
}
parenLevel = newParenLevel;
if (inTargetDefinition) {
const size_t endOfLine = (newParentLevel == 0) ? line.find_last_of(')')
: 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" };
Copy link
Collaborator

@dascandy dascandy Oct 22, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@maikelvdh maikelvdh Oct 23, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@maikelvdh maikelvdh Oct 23, 2017

Choose a reason for hiding this comment

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

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.

for (auto& cmakeTargetParameter : cmakeTargetParameters) {
int pos = targetLine.find(cmakeTargetParameter.c_str());
if (pos != std::string::npos) {
comp.additionalTargetParameters.insert(cmakeTargetParameter);
}
}
}
if (newParentLevel == 0) {
inTargetDefinition = false;
}
}
parentLevel = newParentLevel;
} while (in.good());
assert(parenLevel == 0 || (printf("final level of parentheses=%d\n", parenLevel), 0));
assert(parentLevel == 0 || (printf("final level of parentheses=%d\n", parentLevel), 0));
}

void LoadFileList(const Configuration& config,
Expand All @@ -327,7 +329,7 @@ void LoadFileList(const Configuration& config,
it.disable_recursion_pending();
#endif
continue;
}
}

if (inferredComponents) AddComponentDefinition(components, parent);

Expand All @@ -352,5 +354,3 @@ void ForgetEmptyComponents(std::unordered_map<std::string, Component *> &compone
++it;
}
}


20 changes: 8 additions & 12 deletions test/CmakeRegenTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ TEST(RegenerateCmakeTargetLinkLibraries_Interface) {
TEST(RegenerateCmakeAddTarget_Interface) {
Component comp("./MyComponent/");
comp.type = "add_library";
comp.additionalTargetParameters = " EXCLUDE_FROM_ALL\n";
comp.additionalTargetParameters = { "EXCLUDE_FROM_ALL" };

Configuration config;

Expand All @@ -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"
Copy link
Collaborator

@dascandy dascandy Oct 22, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@maikelvdh maikelvdh Oct 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

")\n"
"\n");

Expand All @@ -326,7 +325,7 @@ TEST(RegenerateCmakeAddTarget_Interface) {
TEST(RegenerateCmakeAddTarget_Library) {
Component comp("./MyComponent/");
comp.type = "add_library";
comp.additionalTargetParameters = " EXCLUDE_FROM_ALL\n";
comp.additionalTargetParameters = { "EXCLUDE_FROM_ALL" };

Configuration config;

Expand All @@ -339,11 +338,10 @@ TEST(RegenerateCmakeAddTarget_Library) {
};

const std::string expectedOutput(
"add_library(${PROJECT_NAME} STATIC\n"
"add_library(${PROJECT_NAME} EXCLUDE_FROM_ALL\n"
" Analysis.cpp\n"
" Analysis.h\n"
" main.cpp\n"
" EXCLUDE_FROM_ALL\n"
")\n"
"\n");

Expand All @@ -356,7 +354,7 @@ TEST(RegenerateCmakeAddTarget_Library) {
TEST(RegenerateCmakeAddTarget_LibraryAlias) {
Component comp("./MyComponent/");
comp.type = "add_library_alias";
comp.additionalTargetParameters = " EXCLUDE_FROM_ALL\n";
comp.additionalTargetParameters = { "EXCLUDE_FROM_ALL" };

Configuration config;
config.addLibraryAliases.insert("add_library_alias");
Expand All @@ -370,11 +368,10 @@ TEST(RegenerateCmakeAddTarget_LibraryAlias) {
};

const std::string expectedOutput(
"add_library_alias(${PROJECT_NAME} STATIC\n"
"add_library_alias(${PROJECT_NAME} EXCLUDE_FROM_ALL\n"
" Analysis.cpp\n"
" Analysis.h\n"
" main.cpp\n"
" EXCLUDE_FROM_ALL\n"
")\n"
"\n");

Expand All @@ -387,7 +384,7 @@ TEST(RegenerateCmakeAddTarget_LibraryAlias) {
TEST(RegenerateCmakeAddTarget_Executable) {
Component comp("./MyComponent/");
comp.type = "add_executable";
comp.additionalTargetParameters = " EXCLUDE_FROM_ALL\n";
comp.additionalTargetParameters = { "EXCLUDE_FROM_ALL" };

Configuration config;

Expand All @@ -400,11 +397,10 @@ TEST(RegenerateCmakeAddTarget_Executable) {
};

const std::string expectedOutput(
"add_executable(${PROJECT_NAME}\n"
"add_executable(${PROJECT_NAME} EXCLUDE_FROM_ALL\n"
" Analysis.cpp\n"
" Analysis.h\n"
" main.cpp\n"
" EXCLUDE_FROM_ALL\n"
")\n"
"\n");

Expand Down
26 changes: 26 additions & 0 deletions test/InputTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,29 @@ TEST(Input_Aliases)
}
}
}

TEST(Input_AdditionalTargetParameters)
{
TemporaryWorkingDirectory workDir(name);

{
streams::ofstream out(workDir() / "CMakeLists.txt");
out << "project(TestProject)\n"
<< "add_library(${PROJECT_NAME} SHARED EXCLUDE_FROM_ALL somesourcefile.cpp)\n";
}

Configuration config;

std::unordered_map<std::string, Component*> components;
std::unordered_map<std::string, File> files;

LoadFileList(config, components, files, workDir(), true, false);

ASSERT(components.size() == 1);

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

@dascandy dascandy Oct 22, 2017

Choose a reason for hiding this comment

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

Why do you assert on the order?

Copy link
Member Author

@maikelvdh maikelvdh Oct 23, 2017

Choose a reason for hiding this comment

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

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

}
}