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

Multiple flags are correctly read into VS projects, added support for preprocessor definitions in addons #214

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

Conversation

brinoausrino
Copy link

Before the project generator only saved the last flag of each type and overwrote the previous ones. This could be fixed by overwriting the first child of the note but not the not itself

from
additionalOptions.first_child().set_value
to
additionalOptions.set_value((std::string(additionalOptions.value()) + " " + cflag).c_str());

Addionally I added the support for preprocessor definitions in addons in VS projects.

@ofZach
Copy link
Contributor

ofZach commented Aug 28, 2019

thanks! looks like a helpful change

@arturoc
Copy link
Member

arturoc commented Aug 29, 2019

the way overwriting works is by using += if you don't want to overwrite and just = if you want to overwrite, the syntax is based on makefiles syntax and allows to for example overwrite the defaults that are parsed from the file system which is quite useful for certain cases. or i'm understanding this wrong?

Also aren't preprocessir definitions the same as the already present ADDON_DEFINES?

@brinoausrino
Copy link
Author

Yes, this is how it should work. But the project generator for vs overwrites the setting even, when using +=. So I did the fix, that it works how it should.

Yes, you are right. I did not get it that the ADDON_DEFINES should be used for the preprocessor. The had a small bug as well. Fixed it and removed the separate preprocessor definitions

if(variable == "ADDON_CFLAGS"){
addReplaceStringVector(cflags,value,"",addToValue);
}

if (variable == "ADDON_PREPROCESSOR_DEFINITIONS") {
Copy link
Member

Choose a reason for hiding this comment

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

this reference to ADDON_PREPROCESSOR_DEFINITIONS should go as well

Copy link
Author

Choose a reason for hiding this comment

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

so, then adding ADDON_PREPROCESSOR_DEFINITIONS is useful?

Since I changed
additionalOptions = items[i].node().child("ClCompile").child("AdditionalOptions");
for
additionalOptions = items[i].node().child("ClCompile").child("PreprocessorDefinitions");

Should we leave it like this or should we use the seperate preprocessor flag?

Copy link
Member

Choose a reason for hiding this comment

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

no sorry, what i meant is it should go away

Copy link
Author

Choose a reason for hiding this comment

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

already is in the latest commit

@@ -310,7 +315,7 @@ void ofAddon::parseVariableValue(string variable, string value, bool addToValue,
}

if(variable == "ADDON_DATA"){
addReplaceStringVector(data,value,"",addToValue);
addReplaceStringVector(data,value,addonRelPath,addToValue);
Copy link
Member

Choose a reason for hiding this comment

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

what is this? doesn't seem related to any of the problems explained in the PR no?

Copy link
Author

Choose a reason for hiding this comment

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

No, sorry I don't really know why that happened.

Copy link
Author

Choose a reason for hiding this comment

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

ok, fixed it.

ofxProjectGenerator/src/addons/ofAddon.h Outdated Show resolved Hide resolved
@arturoc
Copy link
Member

arturoc commented Aug 30, 2019

not sure what's going on but all the changes you've marked as resolved aren't really there. perhaps you forgot to push?

@brinoausrino
Copy link
Author

not sure what's going on but all the changes you've marked as resolved aren't really there. perhaps you forgot to push?

Did I oversee something (do I need to make a new pull request, or does this work automatically)? The latest commit should resolve the addReplaceStringVector() thing:

37b5cbf

The one before the preprocessor issue

3ff92b5

You have any idea?

@arturoc
Copy link
Member

arturoc commented Aug 30, 2019

those commits are already in, you don't need to create a new PR i think you are just missing some bits that are still referring to addon preprocessor define. look carefully at my annotations and the line numbers

@brinoausrino
Copy link
Author

sorry arturo, I was a little confused about the codeline. should be fixed now.

@@ -45,6 +45,7 @@ class ofAddon {
std::vector < std::string > frameworks; // osx only
std::vector < std::string > data;
std::vector < std::string > defines;
std::vector < std::string > preprocessorDefinitions; // vs only
Copy link
Member

Choose a reason for hiding this comment

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

this one is still missing, it should go away to. there should be no changes in ofAddon.h only on visualStudioProject

Copy link
Author

Choose a reason for hiding this comment

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

excuse the inconvenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants