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

Get 'compileas' working when using a 'filter "files: ..."' scope. #1040

Merged
merged 1 commit into from
May 29, 2018

Conversation

ratzlaff
Copy link
Contributor

@ratzlaff ratzlaff commented Apr 9, 2018

Extends compileas to be used in file filters

  project "main"
      language "C++"

      files  { "src/file.c" }
      filter { "files:src/file.c" }
          compileas "C++"

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.

It seems like there might be some unrelated changes in the gmake action. I'm not sure if there's much value in adding support for this to gmake given that gmake2 is intended to replace it.

Also, your new gmake2 tests aren't as comprehensive as the gmake tests, could you flesh them out the same way?

$(SILENT) mkdir -p $(OBJDIR)
else
$(SILENT) mkdir $(subst /,\\,$(OBJDIR))
endif
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be an unrelated change? I think this is how it used to be but it was causing issues when running multiple jobs.

@tvandijck
Copy link
Contributor

LGTM, I'm deferring to someone else to review/test this however.

@samsinsane
Copy link
Member

To be honest, everything except for the gmake changes LGTM. I don't understand enough about the gmake generator to know if something will break - I'll mark this as approved, but hopefully @starkos or @TurkeyMan can chime in on the gmake changes. (For what it's worth, the gmake2 changes make sense!)

Copy link
Member

@starkos starkos left a comment

Choose a reason for hiding this comment

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

The gmake changes look reasonable. I do think the time has come to consider deprecating gmake for gmake2, but I'll open a separate issue for that.

@tdesveauxPKFX
Copy link
Contributor

Everyone seems to be ok with this and I have nothing to say either.
Once you rebase on master, I (or someone else) will merge.

@ratzlaff
Copy link
Contributor Author

@tdesveauxPKFX rebased as requested!

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.

5 participants