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 MVP implementation of overriding options. #1457

Merged
merged 7 commits into from Apr 3, 2017
Merged

Add MVP implementation of overriding options. #1457

merged 7 commits into from Apr 3, 2017

Conversation

jpakkane
Copy link
Member

@jpakkane jpakkane commented Mar 10, 2017

Allow (eventually) overriding any options. Uses include:

  • disable werror on some targets
  • set language standard per target (e.g. C++03 and C++11 in the same build)
  • disable unity building targets that are not unity buildable
  • and so on

This is a rough idea, refinement comments welcome.

@jpakkane jpakkane force-pushed the overrides branch 5 times, most recently from 602f4d6 to b414e51 Compare Mar 12, 2017
run_unittests.py Outdated
@@ -1189,6 +1189,25 @@ def test_installed_modes(self):
self.assertEqual(0, statf.st_uid)


def test_cpp_std_override(self):
Copy link
Member

@ignatenkobrain ignatenkobrain Mar 12, 2017

Choose a reason for hiding this comment

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

[flake8]

  • [E303] too many blank lines (2)

@@ -6,7 +6,7 @@

# http://www.apache.org/licenses/LICENSE-2.0

# Unless required by applicable law or agreed to in writing, software
# Unless required by applicable law or agreed to in writingget_, software
Copy link
Contributor

@absmall absmall Mar 14, 2017

Choose a reason for hiding this comment

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

Typo here

@nirbheek
Copy link
Member

@nirbheek nirbheek commented Mar 27, 2017

I like this PR overall, what's blocking it from merging?

@jpakkane
Copy link
Member Author

@jpakkane jpakkane commented Mar 27, 2017

Lack of review.

@nirbheek
Copy link
Member

@nirbheek nirbheek commented Mar 27, 2017

Okay, I'll do a proper review of this tomorrow.

@@ -1882,6 +1884,13 @@ def generate_single_compile(self, target, outfile, src, is_generated=False, head
raise AssertionError('BUG: sources should not contain headers {!r}'.format(src))
if isinstance(src, RawFilename) and src.fname.endswith('.h'):
raise AssertionError('BUG: sources should not contain headers {!r}'.format(src.fname))
extra_orderdeps = []
Copy link
Member

@ignatenkobrain ignatenkobrain Mar 27, 2017

Choose a reason for hiding this comment

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

[flake8]

  • [F841] local variable 'extra_orderdeps' is assigned to but never used

Copy link
Member

@nirbheek nirbheek left a comment

The rest of it looks good to me. The nice part about this rework is that at most some options won't be properly overriden per-target, which shouldn't cause regressions and can be fixed when reported.

if isinstance(value, str):
if not value.startswith('['):
raise MesonException('Valuestring does not define an array: ' + value)
newvalue = eval(value, {}, {}) # Yes, it is unsafe.
Copy link
Member

@nirbheek nirbheek Mar 28, 2017

Choose a reason for hiding this comment

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

import ast
newvalue = ast.literal_eval(value)


class OptionOverrideProxy:
'''Mimic an option list but transparently override
selected option values.'''
Copy link
Member

@nirbheek nirbheek Mar 28, 2017

Choose a reason for hiding this comment

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

This should start with an indent. Python docs remove leading indentation automatically.

@@ -1899,6 +1908,10 @@ def generate_single_compile(self, target, outfile, src, is_generated=False, head
abs_src = src.fname
else:
abs_src = os.path.join(self.environment.get_build_dir(), src.fname)
elif isinstance(src, mesonlib.File):
Copy link
Member

@nirbheek nirbheek Mar 28, 2017

Choose a reason for hiding this comment

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

Is this needed? We have an else: if isinstance(src, File): below again.

Copy link
Member Author

@jpakkane jpakkane Mar 28, 2017

Choose a reason for hiding this comment

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

It breaks if this is not there. Let's fix it when ripping out RawFile. Weirdly, I thought I already did it ages ago but apparently not.


executable('mustunity', 'one.c', 'two.c')
executable('notunity', 'three.c', 'four.c',
override_options : ['unity=false'])
Copy link
Member

@nirbheek nirbheek Mar 28, 2017

Choose a reason for hiding this comment

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

Should test the other option types too, not just string. And maybe also test overriding options set from various sources: project(), built-in, command-line, meson_options, etc.

Copy link
Member Author

@jpakkane jpakkane Mar 29, 2017

Choose a reason for hiding this comment

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

Added werror test. You can't really override meson_options because these overrides are only used inside builtin targets and those do not look up project options.

@tp-m
Copy link
Member

@tp-m tp-m commented Mar 28, 2017

I can't meaningfully review this, but I am looking forward to seeing this land and using it.

@jpakkane
Copy link
Member Author

@jpakkane jpakkane commented Mar 28, 2017

Updated apart from tests.

@jpakkane
Copy link
Member Author

@jpakkane jpakkane commented Apr 1, 2017

Rebased, should be good now.

@jpakkane jpakkane merged commit 8b73d80 into master Apr 3, 2017
2 of 3 checks passed
@jpakkane jpakkane deleted the overrides branch Apr 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants