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.

Sign up for GitHub

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

Jump to bottom

Go through coreutils.compiler_options.{build.host.target} #4626

Merged
merged 2 commits into from
Feb 4, 2019
Merged

Go through coreutils.compiler_options.{build.host.target} #4626

merged 2 commits into from
Feb 4, 2019

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Dec 12, 2018 •
edited
Loading

Never access environment.properties downstream. Instead use coredata.compiler_options.. This brings the cross and native code paths closer together, since both now use that.

#4772 builds on this to further remove a bunch of conditional code now that the underlying mechanism is the same. It has some CI failures though hence why I didn't do that low-hanging fruit as part of this PR.

Command line options are interpreted just as before, for backwards compatibility. Doing so does introduce some funny conditionals to keep the build and host compiler_options in the native case. In the future, I'd like to change the interpretation of command line options so

  • The logic is cross-agnostic, i.e. there are no conditions affected by
    is_cross_build().

  • Compiler args for both the build and host machines can always be
    controlled by the command line.

  • Compiler args for both machines can always be controlled separately.

@Ericson2314 Ericson2314 changed the title WIP: Never access coreutils.compiler_options---blocked on #4445 WIP: Go through coreutils.compiler_options.{build.host.target}---blocked on #4445 Dec 17, 2018
@Ericson2314 Ericson2314 mentioned this pull request Dec 17, 2018
Merged
@Ericson2314 Ericson2314 changed the title WIP: Go through coreutils.compiler_options.{build.host.target}---blocked on #4445 WIP: Go through coreutils.compiler_options.{build.host.target}---blocked on #4445 and $4645 Dec 17, 2018
@Ericson2314 Ericson2314 changed the title WIP: Go through coreutils.compiler_options.{build.host.target}---blocked on #4445 and $4645 WIP: Go through coreutils.compiler_options.{build.host.target}---blocked on #4445 and #4645 Dec 17, 2018
@Ericson2314 Ericson2314 mentioned this pull request Dec 17, 2018
Closed
3 tasks
@ignatenkobrain
Copy link
Member

Flake8 detected 10 issues on e5c3f55
Visit https://sider.review/gh/repos/19784232/pulls/4626 to review the issues.

posted by Sider

@Ericson2314 Ericson2314 mentioned this pull request Dec 18, 2018
Open
@Ericson2314 Ericson2314 force-pushed the consolidate-properties branch 4 times, most recently from b8865b4 to 9ac92a2 Compare December 20, 2018 06:26
@Ericson2314 Ericson2314 changed the title WIP: Go through coreutils.compiler_options.{build.host.target}---blocked on #4445 and #4645 WIP: Go through coreutils.compiler_options.{build.host.target}---blocked on #4445 Dec 22, 2018
@Ericson2314 Ericson2314 changed the title WIP: Go through coreutils.compiler_options.{build.host.target}---blocked on #4445 WIP: Go through coreutils.compiler_options.{build.host.target} Jan 6, 2019
@Ericson2314
Copy link
Member Author

Ericson2314 commented Jan 6, 2019 •
edited
Loading

@dcbaker @jpakkane now that #4445 has landed, I'd love to discuss the issue of cross-native consistency i discussed in the PR description. Some decision is needed to finish this PR (including get that last test succeeding).

@Ericson2314 Ericson2314 mentioned this pull request Jan 6, 2019
Merged
@Ericson2314 Ericson2314 force-pushed the consolidate-properties branch 2 times, most recently from 8cb6082 to 3b3bf0c Compare January 9, 2019 04:29
ignatenkobrain
ignatenkobrain reviewed Jan 9, 2019
View reviewed changes
mesonbuild/mintro.py Outdated Show resolved Hide resolved
@Ericson2314 Ericson2314 force-pushed the consolidate-properties branch 3 times, most recently from 1e05fe4 to 58f7902 Compare January 14, 2019 05:57
@Ericson2314 Ericson2314 changed the title WIP: Go through coreutils.compiler_options.{build.host.target} WIP: Go through coreutils.compiler_options.{build.host.target}---blocked on #4733 Jan 14, 2019
@Ericson2314 Ericson2314 changed the title WIP: Go through coreutils.compiler_options.{build.host.target}---blocked on #4733 Go through coreutils.compiler_options.{build.host.target}---blocked on #4733 Jan 14, 2019
@Ericson2314
Copy link
Member Author

This PR works and is itself done, but needs to be rebased once #4733 is merged.

@Ericson2314 Ericson2314 mentioned this pull request Jan 14, 2019
Merged
@Ericson2314 Ericson2314 force-pushed the consolidate-properties branch 2 times, most recently from b86b7b8 to 053cd53 Compare January 17, 2019 01:33
@Ericson2314
Copy link
Member Author

@jpakkane can you restart the one CI failure? It's spurious.

@dcbaker thanks again for pinging on IRC. Fixed my mistakes rebasing (or also maybe new tests added since last rebase caught 1 more thing!), and now should be ready for review.

@Ericson2314 Ericson2314 mentioned this pull request Jan 17, 2019
Merged
@Ericson2314 Ericson2314 changed the title Go through coreutils.compiler_options.{build.host.target}---blocked on #4733 Go through coreutils.compiler_options.{build.host.target} Jan 21, 2019
@Ericson2314
Copy link
Member Author

@dcbaker have time to review this too?

@Ericson2314
C# and Java compilers should have is_cross = False
6dbe33d 
All compilers should define this attribute. Probably should change base
class to require this.
@Ericson2314 Ericson2314 force-pushed the consolidate-properties branch 2 times, most recently from 435a27c to cac9c95 Compare February 1, 2019 19:06
dcbaker
dcbaker requested changes Feb 1, 2019
View reviewed changes
Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

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

I've got a few small things I'd like to see fixed, but otherwise this looks good.

mesonbuild/coredata.py Outdated Show resolved Hide resolved
mesonbuild/coredata.py Outdated Show resolved Hide resolved
mesonbuild/coredata.py Outdated Show resolved Hide resolved
mesonbuild/environment.py Outdated Show resolved Hide resolved
mesonbuild/environment.py Outdated Show resolved Hide resolved
mesonbuild/interpreter.py Outdated Show resolved Hide resolved
@Ericson2314
Never access environment.properties downstream
19f81d3 
Instead use coredata.compiler_options.<machine>. This brings the cross
and native code paths closer together, since both now use that.

Command line options are interpreted just as before, for backwards
compatibility. This does introduce some funny conditionals. In the
future, I'd like to change the interpretation of command line options so

 - The logic is cross-agnostic, i.e. there are no conditions affected by
   `is_cross_build()`.

 - Compiler args for both the build and host machines can always be
   controlled by the command line.

 - Compiler args for both machines can always be controlled separately.
@Ericson2314
Copy link
Member Author

@dcbaker OK all your changes are fixed, along with the Sider issues I accidentally reintroduced in the process.

@jpakkane jpakkane merged commit 902aaf2 into mesonbuild:master Feb 4, 2019
@Ericson2314 Ericson2314 deleted the consolidate-properties branch February 4, 2019 22:12
jpakkane added a commit that referenced this pull request Apr 12, 2019
@jpakkane
Revert "Merge pull request #4626 from Ericson2314/consolidate-propert…
f706e42 
…ies"

This reverts commit 902aaf2, reversing
changes made to 59791fc.
jpakkane added a commit that referenced this pull request May 9, 2019
@jpakkane
Revert "Merge pull request #4626 from Ericson2314/consolidate-propert…
00151b8 
…ies"

This reverts commit 902aaf2, reversing
changes made to 59791fc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ignatenkobrain ignatenkobrain ignatenkobrain left review comments

@dcbaker dcbaker dcbaker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Ericson2314 @ignatenkobrain @dcbaker @jpakkane