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

Add new add_project_[link]_args functions #1030

Merged
merged 1 commit into from
Nov 12, 2016

Conversation

thiblahute
Copy link
Contributor

Fixes 979

if cc.get_id() == 'msvc'
add_global_arguments('/DGLOBAL_ARGUMENT', language: 'c')
add_project_arguments('/DPROJECT_OPTION', language: 'c')
add_project_arguments('/DPROJECT_OPTION_1', language: 'c')
Copy link
Member

Choose a reason for hiding this comment

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

This is not required. Just use -DGLOBAL_ARGUMENT etc. MSVC understands that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it was failling with MSVC and suspected this was the reason.


cc = meson.get_compiler('c')
if cc.get_id() == 'msvc'
add_project_arguments('/DSUBPROJECT_OPTION', language: 'c')
Copy link
Member

Choose a reason for hiding this comment

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

Same.

@thiblahute thiblahute force-pushed the project_args branch 2 times, most recently from f7f4460 to 7aa3ef5 Compare November 12, 2016 12:10
@nirbheek
Copy link
Member

Travis build failed because brew update took too long.

Appveyor build is stuck, and the Internet tells me that when this happens you're supposed to cancel the stuck build and kick off a new one. @jpakkane needs to do that.

@nirbheek
Copy link
Member

Please re-push to trigger a rebuild :)

@thiblahute
Copy link
Contributor Author

Please re-push to trigger a rebuild :)

Done.. let's see how it goes :)

@thiblahute thiblahute force-pushed the project_args branch 2 times, most recently from 7242baa to 99e77c8 Compare November 12, 2016 15:25
@nirbheek nirbheek added this to the 0.36.0 milestone Nov 12, 2016
@jpakkane
Copy link
Member

There should be a test that uses C++ and which checks that the project options set for C have not leaked into other languages.

@nirbheek
Copy link
Member

It would also be nice to test that target-specific c/cpp arguments override project arguments.

@@ -1944,6 +1945,7 @@ def generate_link(self, target, outfile, outname, obj_list, linker, extra_args=[
commands = []
if not isinstance(target, build.StaticLibrary):
commands += self.build.get_global_link_args(linker)
commands += self.build.get_project_link_args(linker, target.subproject)
Copy link
Member

Choose a reason for hiding this comment

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

At least -L arguments should be the other way around. The first one is used so for those project link args should be before global ones. But what about other linker arguments? I think that for other ones the last one is the deciding one. Maybe we should just stick to this order and fix things if they actually break in the wild?

Copy link
Member

Choose a reason for hiding this comment

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

What we should be doing is gathering all the -L arguments from all sources into one OrderedDict and then generating the command from that. What you're pointing out is already a problem with our linker argument list since global link -L arguments override target-specific ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jpakkane
Copy link
Member

Args froze is not tested at all (I thought I had written a test for that but grepping says no). This needs a failing test that rougly does the following:

  • add project arg
  • define a build target (hello.c or the like)
  • add a second project arg

Apart from these two fixes this looks good.

@thiblahute
Copy link
Contributor Author

There should be a test that uses C++ and which checks that the project options set for C have not leaked into other languages.

Done

Args froze is not tested at all (I thought I had written a test for that but grepping says no). This needs a failing test that rougly does the following:

Done

@jpakkane jpakkane merged commit 85a0cd7 into mesonbuild:master Nov 12, 2016
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