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

CMake fixes #1725

Merged
merged 2 commits into from Nov 17, 2017

Conversation

Projects
None yet
5 participants
@SoapGentoo
Contributor

SoapGentoo commented May 25, 2017

No description provided.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel May 25, 2017

Member

I'm dubious about f07f703, as those headers are included using the <> syntax which is supposedly what -isystem is supposed to be for if I recall correctly.

Member

CelticMinstrel commented May 25, 2017

I'm dubious about f07f703, as those headers are included using the <> syntax which is supposedly what -isystem is supposed to be for if I recall correctly.

@SoapGentoo

This comment has been minimized.

Show comment
Hide comment
@SoapGentoo
Contributor

SoapGentoo commented May 26, 2017

@loonycyborg

This comment has been minimized.

Show comment
Hide comment
@loonycyborg

loonycyborg May 26, 2017

Member

This issue seems to apply only to using -isystem /usr/include. Using it for other directories should still be fine.

Member

loonycyborg commented May 26, 2017

This issue seems to apply only to using -isystem /usr/include. Using it for other directories should still be fine.

@SoapGentoo

This comment has been minimized.

Show comment
Hide comment
@SoapGentoo

SoapGentoo May 26, 2017

Contributor

Ok, -isystem added back

Contributor

SoapGentoo commented May 26, 2017

Ok, -isystem added back

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg May 26, 2017

Contributor

-isystem is fine and thanks for adding it back in.

@loonycyborg: The issue is not with /usr/include, per se; it is that, coupled with #include-next (as found in stdio.h and math.h), they were just the simplest examples to reproduce it.

Wesnoth's use of -isystem is consistent with the meaning and purpose of the cited documentation; we do NOT want so many diagnostic messages from upstream libraries and treating them as system headers achieves that goal. This is the entire reason GCC gives us the compiler option.

Contributor

GregoryLundberg commented May 26, 2017

-isystem is fine and thanks for adding it back in.

@loonycyborg: The issue is not with /usr/include, per se; it is that, coupled with #include-next (as found in stdio.h and math.h), they were just the simplest examples to reproduce it.

Wesnoth's use of -isystem is consistent with the meaning and purpose of the cited documentation; we do NOT want so many diagnostic messages from upstream libraries and treating them as system headers achieves that goal. This is the entire reason GCC gives us the compiler option.

@GregoryLundberg

Do not change whitespace without good reason. This commit simply changes the target name, but that fact is hard to see given the spurious changes.

The change seems incomplete. While changing the target name from "test" to "wesnoth_test" may be a good idea, it probably breaks other tools such as the run_wml_test script.

This is a major change and should update the change log.

@GregoryLundberg

Each commit should be a single concept. It appears, here, you're mixing a change to how toolchain parts are references with a change to avoid spurious changes to the source tree when performing an out-of-tree build. These should be separate commits.

Is there a specific reason for requiring an upgrade from CMake 2.6 to 2.8.5? Can you not make these changes without forcing a bump in the minimum toolset requirements?

if(win32) else(win32) endif(win32) is confusing. else(win32) implies the following only applies when on Windows when, in fact, is only applies when NOT on Windows.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Jul 1, 2017

Member

Also note that changing the name of the output file in only one toolchain isn't really a good idea. Both scons and cmake should produce a binary with the same name.

Member

CelticMinstrel commented Jul 1, 2017

Also note that changing the name of the output file in only one toolchain isn't really a good idea. Both scons and cmake should produce a binary with the same name.

@CelticMinstrel CelticMinstrel added this to the 1.13.9 milestone Jul 26, 2017

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Sep 2, 2017

Member

Either we merge or close this.

Member

Vultraz commented Sep 2, 2017

Either we merge or close this.

@SoapGentoo

This comment has been minimized.

Show comment
Hide comment
@SoapGentoo

SoapGentoo Sep 2, 2017

Contributor

I'll work on it

Contributor

SoapGentoo commented Sep 2, 2017

I'll work on it

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Sep 2, 2017

Contributor

I don't get why the bump in minimum CMake version, and why the change from configurable directory names to hard-coded names.

A number of the changes are just white-space and should be elimiated.

The change from "test" to "wesnoth-test" is probably a good idea but all it does is suppress a warning when configuring CMake. It requires a lot more than simply changing the executable name.

@Vultraz i recommend pushing this off to 1.13.10

Contributor

GregoryLundberg commented Sep 2, 2017

I don't get why the bump in minimum CMake version, and why the change from configurable directory names to hard-coded names.

A number of the changes are just white-space and should be elimiated.

The change from "test" to "wesnoth-test" is probably a good idea but all it does is suppress a warning when configuring CMake. It requires a lot more than simply changing the executable name.

@Vultraz i recommend pushing this off to 1.13.10

@SoapGentoo

This comment has been minimized.

Show comment
Hide comment
@SoapGentoo

SoapGentoo Sep 2, 2017

Contributor

DATAROOTDIR is a non-standard, non-idiomatic variable that is project internal. CMAKE_INSTALL_DATAROOTDIR is a Kitware-supported, standardised and configurable variable for which every distribution has hooks. Using the same variables everywhere is 1) easier for everyone 2) makes maintenance easier 3) and does not require us to hack build systems.

Contributor

SoapGentoo commented Sep 2, 2017

DATAROOTDIR is a non-standard, non-idiomatic variable that is project internal. CMAKE_INSTALL_DATAROOTDIR is a Kitware-supported, standardised and configurable variable for which every distribution has hooks. Using the same variables everywhere is 1) easier for everyone 2) makes maintenance easier 3) and does not require us to hack build systems.

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Sep 2, 2017

Contributor

I was referring to changes which switched from using a {variable} to using {another-variable}/pathname. .../html, or example, presumes html exists and is the desired name.

Contributor

GregoryLundberg commented Sep 2, 2017

I was referring to changes which switched from using a {variable} to using {another-variable}/pathname. .../html, or example, presumes html exists and is the desired name.

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Nov 10, 2017

Contributor

#2167 changed the target name and executable from 'test' to 'boost_unit_tests'
#2168 removed CMP005

With those both being merged to wesnoth/master CMake should run with no errors and no warnings when generating the Makefile.

I have a local subject branch for #1725 which resolves the merge conflicts and eliminates the spurious changes. I could push this up to complete work on this PR. I don't, however, see the point. It means a change to the build process for all packagers using CMake. I attempted to ask on the Forums, but garnered no responses.

My choice, at this point, is to simply close this PR, or close it in favor of my branch.

Given there seems to be no interest in the remaining changes, unless someone objects, I will be closing this PR and deleting my local branch, discarding the remaining changes, once 1.13.11 (or 1.14 RC1) tags.

Contributor

GregoryLundberg commented Nov 10, 2017

#2167 changed the target name and executable from 'test' to 'boost_unit_tests'
#2168 removed CMP005

With those both being merged to wesnoth/master CMake should run with no errors and no warnings when generating the Makefile.

I have a local subject branch for #1725 which resolves the merge conflicts and eliminates the spurious changes. I could push this up to complete work on this PR. I don't, however, see the point. It means a change to the build process for all packagers using CMake. I attempted to ask on the Forums, but garnered no responses.

My choice, at this point, is to simply close this PR, or close it in favor of my branch.

Given there seems to be no interest in the remaining changes, unless someone objects, I will be closing this PR and deleting my local branch, discarding the remaining changes, once 1.13.11 (or 1.14 RC1) tags.

@SoapGentoo

This comment has been minimized.

Show comment
Hide comment
@SoapGentoo

SoapGentoo Nov 17, 2017

Contributor

@GregoryLundberg how would you like me to commit the changes, in what order?

I agree that the workflow changes for packagers, but it changes to a uniform way of doing things. Most distros already have hooks in place for GNUInstallDirs variables. The GNUInstallDirs module is the only way to let users change the dirs without each downstream reinventing the vars.

Contributor

SoapGentoo commented Nov 17, 2017

@GregoryLundberg how would you like me to commit the changes, in what order?

I agree that the workflow changes for packagers, but it changes to a uniform way of doing things. Most distros already have hooks in place for GNUInstallDirs variables. The GNUInstallDirs module is the only way to let users change the dirs without each downstream reinventing the vars.

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Nov 17, 2017

Contributor

My branch has what remains of your changes, sans spurious changes; rebased as of a week or so ago. Feel free to grab it and push it over your branch; or hand-apply the changes.

If you feel the white-space changes are important, please move them to a separate commit.

I can see what you're doing and don't see a problem with it except nobody seems to care.

So the question is merge a breaking change or not. I'm on the fence. So, if you want to stand up and state that as one of the Gentoo packagers, you want the change, I'll take that as 1 yea, 0 nay and N abstentions, and merge to see who complains. Be aware 1.13.11 (1.14 beta 3) is scheduled to tag Sunday. So if you want it, either tell me to merge my branch or update and push yours in the next day to day-and-a-half.

Contributor

GregoryLundberg commented Nov 17, 2017

My branch has what remains of your changes, sans spurious changes; rebased as of a week or so ago. Feel free to grab it and push it over your branch; or hand-apply the changes.

If you feel the white-space changes are important, please move them to a separate commit.

I can see what you're doing and don't see a problem with it except nobody seems to care.

So the question is merge a breaking change or not. I'm on the fence. So, if you want to stand up and state that as one of the Gentoo packagers, you want the change, I'll take that as 1 yea, 0 nay and N abstentions, and merge to see who complains. Be aware 1.13.11 (1.14 beta 3) is scheduled to tag Sunday. So if you want it, either tell me to merge my branch or update and push yours in the next day to day-and-a-half.

@SoapGentoo

This comment has been minimized.

Show comment
Hide comment
@SoapGentoo

SoapGentoo Nov 17, 2017

Contributor

@GregoryLundberg I've cleaned it up and rebased on master. I've also fixed the relative handling on Windows to be much simpler. Could you have another look please?

Contributor

SoapGentoo commented Nov 17, 2017

@GregoryLundberg I've cleaned it up and rebased on master. I've also fixed the relative handling on Windows to be much simpler. Could you have another look please?

@GregoryLundberg

Travis failures due to CMP005 removal.

Show outdated Hide outdated CMakeLists.txt
Show outdated Hide outdated CMakeLists.txt
@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Nov 17, 2017

Contributor

Add a separate commit to update the changelog in the (probably vain) hope the packagers read it if this breaks their workflows :P.

Rename my CI section to include packaging.
Add a comment about the CMake minimum requirements changing.
Then add a brief comment or two about what you've done.

When Travis finishes, if there are no more issues, I'll merge.

Contributor

GregoryLundberg commented Nov 17, 2017

Add a separate commit to update the changelog in the (probably vain) hope the packagers read it if this breaks their workflows :P.

Rename my CI section to include packaging.
Add a comment about the CMake minimum requirements changing.
Then add a brief comment or two about what you've done.

When Travis finishes, if there are no more issues, I'll merge.

@SoapGentoo

This comment has been minimized.

Show comment
Hide comment
@SoapGentoo

SoapGentoo Nov 17, 2017

Contributor

@GregoryLundberg before I push, do you mind me renaming the changelog to ChangeLog? The latter is the GNU convention, which is nice, as it gets auto-installed on Gentoo and other distros.

Contributor

SoapGentoo commented Nov 17, 2017

@GregoryLundberg before I push, do you mind me renaming the changelog to ChangeLog? The latter is the GNU convention, which is nice, as it gets auto-installed on Gentoo and other distros.

@SoapGentoo

This comment has been minimized.

Show comment
Hide comment
@SoapGentoo

SoapGentoo Nov 17, 2017

Contributor

Never mind, renaming would trickle a cascade of massive packaging changes.

Contributor

SoapGentoo commented Nov 17, 2017

Never mind, renaming would trickle a cascade of massive packaging changes.

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Nov 17, 2017

Contributor

Travis is having problems finding the wesnoth and boost_unit_tests executable files.
I'm looking at it and think it's something your CMake changes changed which need to be reflected into utils .. I want to let Travis fully finish (which will be a couple more hours) before I dig into it. But if you see the issue (and do NOT break SCons) go ahead and push another update.

Contributor

GregoryLundberg commented Nov 17, 2017

Travis is having problems finding the wesnoth and boost_unit_tests executable files.
I'm looking at it and think it's something your CMake changes changed which need to be reflected into utils .. I want to let Travis fully finish (which will be a couple more hours) before I dig into it. But if you see the issue (and do NOT break SCons) go ahead and push another update.

@SoapGentoo

This comment has been minimized.

Show comment
Hide comment
@SoapGentoo

SoapGentoo Nov 17, 2017

Contributor

@GregoryLundberg my gut feeling tells me its the removal of

set(EXECUTABLE_OUTPUT_PATH "${CMAKE_BINARY_DIR}" [...])

The reason why I removed this is because people tend to rely too much on build tree details, which are ultimately an implementation detail of the build system and something that should not be relied upon. I would recommend installing it to some temp dir and running it from there. This also due to the use of the full data path in the executable. Your ideas?

Contributor

SoapGentoo commented Nov 17, 2017

@GregoryLundberg my gut feeling tells me its the removal of

set(EXECUTABLE_OUTPUT_PATH "${CMAKE_BINARY_DIR}" [...])

The reason why I removed this is because people tend to rely too much on build tree details, which are ultimately an implementation detail of the build system and something that should not be relied upon. I would recommend installing it to some temp dir and running it from there. This also due to the use of the full data path in the executable. Your ideas?

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Nov 17, 2017

Contributor

Sounds like a strong probability. If you don't want to dirty up this PR, as a test make a [WIP, TESTING] PR (so we know not to merge it, with these changes and that change and we can compare results in a couple hours when both are through Travis.

Contributor

GregoryLundberg commented Nov 17, 2017

Sounds like a strong probability. If you don't want to dirty up this PR, as a test make a [WIP, TESTING] PR (so we know not to merge it, with these changes and that change and we can compare results in a couple hours when both are through Travis.

SoapGentoo added some commits Nov 17, 2017

Use GNUInstallDirs to specify directories
* GNUInstallDirs is the only Kitware-supported
  way to change the default directories. Most
  distributions have hooks for changing these
  directories, which makes integrating wesnoth
  easier and more consistent with the rest of
  the CMake ecosystem.
* Make build system perfectly out-of-source
  compatible. The build system should never
  touch files in the source tree.
Update changelog
* Detail the `GNUInstallDirs` changes
* Non-Windows builds now use the absolute path for all
  data paths. This is less brittle, as relative path
  lookup in Unix always depends on the current value of
  the PWD environmental variable.
@SoapGentoo

This comment has been minimized.

Show comment
Hide comment
@SoapGentoo

SoapGentoo Nov 17, 2017

Contributor

@GregoryLundberg brought previous EXECUTABLE_OUTPUT_PATH back, let's see now

Contributor

SoapGentoo commented Nov 17, 2017

@GregoryLundberg brought previous EXECUTABLE_OUTPUT_PATH back, let's see now

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Nov 17, 2017

Contributor

Waiting for Windows VS 2013, 2015 and 2017, and macOS to finish up but there's green lights down all Linux checks. The remaining checks should all pass since they don't use CMake.

If you want to take a while and consider alternatives to the last change, fine; otherwise looks like this PR is good to go.

Contributor

GregoryLundberg commented Nov 17, 2017

Waiting for Windows VS 2013, 2015 and 2017, and macOS to finish up but there's green lights down all Linux checks. The remaining checks should all pass since they don't use CMake.

If you want to take a while and consider alternatives to the last change, fine; otherwise looks like this PR is good to go.

@SoapGentoo

This comment has been minimized.

Show comment
Hide comment
@SoapGentoo

SoapGentoo Nov 17, 2017

Contributor

Nah, removing EXECUTABLE_OUTPUT_PATH will be done in a separate PR, since it will require touching travis and a bunch of other files.

Contributor

SoapGentoo commented Nov 17, 2017

Nah, removing EXECUTABLE_OUTPUT_PATH will be done in a separate PR, since it will require touching travis and a bunch of other files.

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Nov 17, 2017

Contributor

Then I will merge in a few hours when I see the CI systems pass instead of just knowing/thinking they will :)

Contributor

GregoryLundberg commented Nov 17, 2017

Then I will merge in a few hours when I see the CI systems pass instead of just knowing/thinking they will :)

@SoapGentoo

This comment has been minimized.

Show comment
Hide comment
@SoapGentoo

SoapGentoo Nov 17, 2017

Contributor

This has been in the works for 6 months (due to my timing out), I can wait another day if that is what it takes 😉

Contributor

SoapGentoo commented Nov 17, 2017

This has been in the works for 6 months (due to my timing out), I can wait another day if that is what it takes 😉

@SoapGentoo

This comment has been minimized.

Show comment
Hide comment
@SoapGentoo

SoapGentoo Nov 17, 2017

Contributor

The job exceeded the maximum time limit for jobs, and has been terminated. 😢

Contributor

SoapGentoo commented Nov 17, 2017

The job exceeded the maximum time limit for jobs, and has been terminated. 😢

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Nov 17, 2017

Contributor

macOS timed out waiting for a Docker to free up .. restarted. np

Contributor

GregoryLundberg commented Nov 17, 2017

macOS timed out waiting for a Docker to free up .. restarted. np

@GregoryLundberg GregoryLundberg merged commit 509aa5a into wesnoth:master Nov 17, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Nov 17, 2017

Contributor

Boom. Done.

Contributor

GregoryLundberg commented Nov 17, 2017

Boom. Done.

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Nov 17, 2017

Contributor

Well crap.

You moved all the translations (*.mo) from the translations folder to the po folder.

Two problems:

  1. SCons still puts them in translations
  2. The PO folder already has the source files (*.po)
  3. Given the above, getting .gitignore right is a pain

Ooops that's three problems.

While (1) is fixable, (2) means the packagers will need to pick apart the po directory folder/file by folder/file.

My suggestion is to put the *.po files back in the translations folder.

Contributor

GregoryLundberg commented Nov 17, 2017

Well crap.

You moved all the translations (*.mo) from the translations folder to the po folder.

Two problems:

  1. SCons still puts them in translations
  2. The PO folder already has the source files (*.po)
  3. Given the above, getting .gitignore right is a pain

Ooops that's three problems.

While (1) is fixable, (2) means the packagers will need to pick apart the po directory folder/file by folder/file.

My suggestion is to put the *.po files back in the translations folder.

GregoryLundberg added a commit that referenced this pull request Nov 17, 2017

Git: do NOT ignore any MO translations in the po folder.
PR #1725 moved the compiled translations from the translations directory to the po directory.

I would have noticed sooner if Git had not been told to ignore virtually all MO files.
@SoapGentoo

This comment has been minimized.

Show comment
Hide comment
@SoapGentoo

SoapGentoo Nov 18, 2017

Contributor

@GregoryLundberg I'll fix it tomorrow. The reason I changed it (and the installed image looked good) is as the previous solution seemed to pollute the source tree, which is generally something that should be avoided.

Contributor

SoapGentoo commented Nov 18, 2017

@GregoryLundberg I'll fix it tomorrow. The reason I changed it (and the installed image looked good) is as the previous solution seemed to pollute the source tree, which is generally something that should be avoided.

@SoapGentoo SoapGentoo deleted the SoapGentoo:cmake-fixes branch Nov 18, 2017

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Nov 18, 2017

Contributor

Well, one developer is learning something new about fixing merge conflicts, and I learned my build farm had a glaring hole so I'm tearing it down and rebuilding the scripts. I've been meaning to do it for a couple years, anyway.

I think:

  • First, a PR to revert to use translations, then

  • create an Issue and PR to rename po to po-source or something like that, eliminate translations directory and build localisations in po directory. Then we can poll a few other developers into the discussion.

Contributor

GregoryLundberg commented Nov 18, 2017

Well, one developer is learning something new about fixing merge conflicts, and I learned my build farm had a glaring hole so I'm tearing it down and rebuilding the scripts. I've been meaning to do it for a couple years, anyway.

I think:

  • First, a PR to revert to use translations, then

  • create an Issue and PR to rename po to po-source or something like that, eliminate translations directory and build localisations in po directory. Then we can poll a few other developers into the discussion.

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Nov 18, 2017

Member

I just remembered certain UMC have their own translations in their own translations/ folders. Changing where the game looks for them could potentially mean all UMC translations break, unless we can add some fallback so the game checks in the old location too. But that seems messy and unnecessary. Suggest reverting the translations directory change.

Member

Vultraz commented Nov 18, 2017

I just remembered certain UMC have their own translations in their own translations/ folders. Changing where the game looks for them could potentially mean all UMC translations break, unless we can add some fallback so the game checks in the old location too. But that seems messy and unnecessary. Suggest reverting the translations directory change.

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Nov 18, 2017

Contributor

For reference:

To be orthogonal with SCons, CMake MUST build the translations in the source-kit directory ~wesnoth/translations, no matter if this is an in-tree or out-of-tree build.

Yes, all possible build workloads dirty up the source kit.

Contributor

GregoryLundberg commented Nov 18, 2017

For reference:

To be orthogonal with SCons, CMake MUST build the translations in the source-kit directory ~wesnoth/translations, no matter if this is an in-tree or out-of-tree build.

Yes, all possible build workloads dirty up the source kit.

@SoapGentoo

This comment has been minimized.

Show comment
Hide comment
@SoapGentoo

SoapGentoo Nov 18, 2017

Contributor

@GregoryLundberg are you in the IRC channel on freenode?

Contributor

SoapGentoo commented Nov 18, 2017

@GregoryLundberg are you in the IRC channel on freenode?

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Nov 18, 2017

Member

He occasionally is, under the names tad or tad_carlucci.

Member

Vultraz commented Nov 18, 2017

He occasionally is, under the names tad or tad_carlucci.

GregoryLundberg added a commit that referenced this pull request Nov 30, 2017

Git: do NOT ignore any MO translations in the po folder.
PR #1725 moved the compiled translations from the translations directory to the po directory.

I would have noticed sooner if Git had not been told to ignore virtually all MO files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment