Skip to content

Convert buildtype to optimization and debug options#3489

Merged
jpakkane merged 21 commits intomasterfrom
buildtypeseparation
Aug 18, 2018
Merged

Convert buildtype to optimization and debug options#3489
jpakkane merged 21 commits intomasterfrom
buildtypeseparation

Conversation

@jpakkane
Copy link
Member

@jpakkane jpakkane commented Apr 27, 2018

The current setup of a couple of different build types is a bit limiting. Ideally you'd want to toggle the optimization levels and "debugness" independent of each other. Some people might want to do releases with -O2 and others with -O3 for example.

This is a very rudimentary fix showing how a small fraction of this would be done. The full setup would go something like this:

  • optimization gets its own option, with values as shown in this MR
  • debug gets a boolean switch
  • buildtype gets a new value of custom
  • setting buildtype sets also the value of the other two to match current behaviour
  • optimization and debug can be set to any value combinations
  • if the combination exactly matches some current build type, the value of builtype is set to that, otherwise it is set to custom

Comments welcome.

@igor-raits
Copy link
Member

Flake8 detected 12 issues on 33d0914
Visit https://sideci.com/gh/19784232/pull_requests/3489 to review the issues.

return ['/Fo' + target]

def get_optimization_args(self, optimization_level):
return msvc_optimization_args[optimization_level]
Copy link
Member

Choose a reason for hiding this comment

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

[Flake8]

[F821] undefined name 'msvc_optimization_args'

'minsize': [], # In a future release: ['-C', 'opt-level=s'],
'debugoptimized': ['-C', 'debuginfo=2'],
'release': [],
'minsize': [],
Copy link
Member

Choose a reason for hiding this comment

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

[Flake8]

[W291] trailing whitespace

return msvc_optimization_args[optimization_level]

def get_debug_args(self, is_debug):
return msvc_debug_args[is_debug]
Copy link
Member

Choose a reason for hiding this comment

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

[Flake8]

[F821] undefined name 'msvc_debug_args'

@igor-raits
Copy link
Member

Flake8 detected 6 issues on 185b5da
Visit https://sideci.com/gh/19784232/pull_requests/3489 to review the issues.

@jpakkane jpakkane force-pushed the buildtypeseparation branch from 185b5da to 98d1385 Compare May 26, 2018 19:02
@jpakkane jpakkane force-pushed the buildtypeseparation branch from 13712e8 to 5ec6b74 Compare June 6, 2018 22:40
@igor-raits
Copy link
Member

Flake8 detected 23 issues on 5ec6b74
Visit https://sideci.com/gh/19784232/pull_requests/3489 to review the issues.

@jpakkane jpakkane force-pushed the buildtypeseparation branch from 5ec6b74 to 04b9089 Compare June 10, 2018 22:00
@jpakkane
Copy link
Member Author

This PR has very large regression potential. We should probably merge it at the very beginning of the next dev cycle so there is enough time to work out the kinks.

@jpakkane jpakkane requested a review from nirbheek June 10, 2018 22:01
@jpakkane jpakkane force-pushed the buildtypeseparation branch from 04b9089 to 250df1d Compare June 10, 2018 22:47
@nirbheek nirbheek self-assigned this Jun 10, 2018
@nirbheek
Copy link
Member

I'll review this after the release.

@jpakkane jpakkane force-pushed the buildtypeseparation branch from 250df1d to a5731db Compare June 18, 2018 19:50
@igor-raits
Copy link
Member

Flake8 detected 10 issues on a5731db
Visit https://sider.review/gh/19784232/pull_requests/3489 to review the issues.

@jpakkane jpakkane force-pushed the buildtypeseparation branch from a5731db to d900c31 Compare June 29, 2018 20:38
@igor-raits
Copy link
Member

Flake8 detected 16 issues on d900c31
Visit https://sider.review/gh/19784232/pull_requests/3489 to review the issues.

@jpakkane jpakkane force-pushed the buildtypeseparation branch from d900c31 to 8f96da0 Compare July 3, 2018 20:50
return msvc_optimization_args[optimization_level]

def get_debug_args(self, is_debug):
return msvc_debug_args[is_debug]
Copy link
Member

Choose a reason for hiding this comment

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

[Flake8]

[F821] undefined name 'msvc_debug_args'

return self.crt_args[crt_val]
assert(crt_val == 'from_buildtype')
# Match what build type flags used to do.
if builtype == 'plain':
Copy link
Member

Choose a reason for hiding this comment

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

[Flake8]

[F821] undefined name 'builtype'

return ['/Fo' + target]

def get_optimization_args(self, optimization_level):
return msvc_optimization_args[optimization_level]
Copy link
Member

Choose a reason for hiding this comment

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

[Flake8]

[F821] undefined name 'msvc_optimization_args'

@nirbheek nirbheek modified the milestones: meson-next, 0.48.0 Jul 7, 2018
@jpakkane jpakkane force-pushed the buildtypeseparation branch from 8f96da0 to 8a5bc80 Compare July 11, 2018 19:59
@jpakkane jpakkane force-pushed the buildtypeseparation branch from 7223b27 to ffde7d0 Compare August 18, 2018 16:12
@nirbheek
Copy link
Member

I mean, a commit message like "Fix Visual Studio." is not very useful, neither is a commit message like "Fix flake8 errors.". In such PRs, either the commits should be squashed and reorganised into logical steps, or squashed into one commit, otherwise it's just noise. The tests also fail between commits, so it's difficult to do a git bisect inside the merge commit.

@jpakkane
Copy link
Member Author

To quote our documentation on this subject:

large branches with many commits should be merged with a merge commit, especially if one of the commits does not pass all tests (which happens in e.g. large and difficult refactorings)

@nirbheek
Copy link
Member

I know, but in this merge all the commits will fail the tests.

@jpakkane
Copy link
Member Author

If you really want me to squash this I can do it. But in my opinion that is worse because you lose all incrementality.

@nirbheek
Copy link
Member

I think preserving history make sense when you have a series of incremental atomic changes. Doing a part of a change (that breaks other things) is incremental in a "work-in-progress" fashion, but not in an "atomic steps" fashion.

I am not deeply invested in this particular PR being squashed or merged, I just want to clarify my thoughts on the matter about when merge commits are useful.

@jpakkane
Copy link
Member Author

Travis failure is unrelated.

@jpakkane jpakkane merged commit d83f771 into master Aug 18, 2018
@jpakkane jpakkane deleted the buildtypeseparation branch August 18, 2018 17:39
stianse pushed a commit to pexip/meson that referenced this pull request Aug 30, 2018
A temporary hack while waiting for the merge of
mesonbuild#3489
@tbodt
Copy link

tbodt commented Sep 22, 2018

The GDBM wrap is now broken since it has an option named debug. I've made a PR to rename the option to gdbm_debug: mesonbuild/gdbm#2

havardgraff pushed a commit to pexip/meson that referenced this pull request Oct 15, 2018
A temporary hack while waiting for the merge of
mesonbuild#3489
havardgraff added a commit to pexip/meson that referenced this pull request Oct 16, 2018
A temporary hack while waiting for the merge of
mesonbuild#3489
havardgraff added a commit to pexip/meson that referenced this pull request Oct 19, 2018
A temporary hack while waiting for the merge of
mesonbuild#3489
stianse pushed a commit to pexip/meson that referenced this pull request Oct 29, 2018
A temporary hack while waiting for the merge of
mesonbuild#3489
stianse pushed a commit to pexip/meson that referenced this pull request Nov 15, 2018
A temporary hack while waiting for the merge of
mesonbuild#3489
havardgraff added a commit to pexip/meson that referenced this pull request Jun 25, 2019
A temporary hack while waiting for the merge of
mesonbuild#3489
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