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

[core] Use 'compileAs' API #771

Merged
merged 1 commit into from
May 25, 2017
Merged

Conversation

tvandijck
Copy link
Contributor

Fix for: #766

Copy link
Member

@samsinsane samsinsane left a comment

Choose a reason for hiding this comment

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

This does fix the issue, but should we instead not use language to determine the project type (vcproj/vcxproj vs csproj)?

@tvandijck
Copy link
Contributor Author

What else is there to determine project type? The only alternative we have is kind.

But I see what you are kind of hinting at?
"what if we just didn't set language at all, and only set it to be explicit when needed..."

I think that is a good idea, but indeed we'd need something else to determine the project type, and then I think we're falling in the same trap as what we tried to avoid with 'language' vs 'dialect'... say for example we did:

project 'foo'
    projecttype 'NativeProject'
    language 'C#'

it wouldn't work, because for C# you need a csproj, not a vcxproj...
So just like

project 'foo'
    language 'C++'
    dialect 'C++14'

the options are related, and need to be validated for correct combinations..
Basically, I'm coming around on the language determines both "language" and "dialect" like we have implemented right now, and it turns out the side effects of the change are quite substantial, and I don't really know how to correctly fix them without introducing API's like this.

@samsinsane
Copy link
Member

I hear what you're saying, the inability to link APIs together does create a bit of an issue when things like this occur. But at the same time, there's really not much difference between the above and:

project 'foo'
  language 'C#'
  compileas 'C++'

Regardless of which direction we go, we're creating an API that is only valid based on another API. We've kind of already got these anyway, frameworkdirs isn't really valid outside of macOS, similarly, the dotnetframework isn't really valid outside of C# (and perhaps C++/CLI? I really don't know). Well, of the things we support, those two APIs are only valid in the cases I mentioned.

I think the tight coupling we've created between "project type" and "language"/"dialect" is a problem, but perhaps compileas is the shortest path forward. I was pondering on the idea that maybe we shift the <CompileAs> element to only emit at the file level when prj.language ~= cfg.language. This could still allow for the original behaviour (C only) to occur at the project level though, but that creates some strange logic and I'm sure someone will eventually make the same change I did and break everything.

I'm happy for this to go through, I did just notice you've called it compileAs though, and it should probably be compileas for consistency and whatnot.

@tvandijck
Copy link
Contributor Author

I think the tight coupling we've created between "project type" and "language"/"dialect" is a problem, but perhaps compileas is the shortest path forward.

It think the compileas is more of a bandaid to the bigger issue. It solves one particular issue, but not the issue I mentioned here: #770

We can merge this in the intermediate time, but I rather think about solving this the right way for the final 5.0 release that @starkos is working towards.

I rather refactor to a language & dialect API, basically reverting the language refactor I recently did going back to a simpler approach, and simply add validation of dialect/language pairs..

@tvandijck
Copy link
Contributor Author

made it lowercase as requested.

@tvandijck
Copy link
Contributor Author

I rather refactor to a language & dialect API...

so, that wouldn't solve a thing either.... for example this would blow up if we added validation:

workspace ‘foo’
  language ‘C++’
  dialect ‘C++11’

project ‘bar’
  language ‘C’

which is not an unthinkable situation (we have tons of those cases already with the "flags {C++11}".
right or wrong, the flags only applied to the C++ language features...

@starkos
Copy link
Member

starkos commented May 3, 2017

The complieas() call was added because…well, because I didn't think things through all the way, clearly. There is no reason for that to exist, you should be able to use language() in its place.

@tvandijck
Copy link
Contributor Author

@starkos not sure I understand... using language for this in its place is not working, it emits this property at all times, because there is no way to not set language.
language(nil) doens't work, since then the project no longer validates, and premake errors out. You are required to set language, which makes this particular vcxproj feature always emit something, which is undesirable, we want the ability to not emit anything. Hence this PR, but again, it feels like a bandaid, specifically for the VS* actions.

@tvandijck tvandijck merged commit 0ec61b4 into premake:master May 25, 2017
@tvandijck tvandijck deleted the compileAs-API branch May 25, 2017 15:43
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