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

Use "MSYS Makefiles" target when buildin on Windows #84

Merged
merged 2 commits into from
Dec 17, 2019
Merged

Use "MSYS Makefiles" target when buildin on Windows #84

merged 2 commits into from
Dec 17, 2019

Conversation

PierreTechoueyres
Copy link
Contributor

Avoid using Visual Studio generator when building on Windows system.
(Fix issue #83).

  • Makefile: conditionally use MSYS generator.

@tarsius
Copy link
Member

tarsius commented Jun 17, 2019

Someone familiar with cmake and windows has to review this.

Unlike before CMAKE_CMD_LINE_OPTIONS is now always set to something, not just on msys. Is that intentional? Please explain in the commit message what the two possible values mean and why we have to use them.

Avoid using Visual Studio generator when building on Windows system by
using MSYS Makefiles generator on Microsoft Windows systems.

(Fix issue #83).

* Makefile: Conditionally use "MSYS Makefiles generator".  Also
  refactor the definition of build type by defining an
  CMAKE_CMD_LINE_OPTIONS variable.
@PierreTechoueyres
Copy link
Contributor Author

Hi @tarsius,
Sorry for the delay. I completely forget this issue.
CMAKE_CMD_LINE_OPTIONS is not an official cmake variable. It's one I defined to avoid the duplication of -DCMAKE_BUILD_TYPE=Debug inside the Makefile.

Could the commit message below be what you expect ?

Avoid using Visual Studio generator when building on Windows system by
using MSYS Makefiles generator on Microsoft Windows systems.

In Microsoft Windows, if Visual Studio is installed, it's the preferred choice of
 cmake before MSYS. But Emacs (and this lib by extension) must be build with
 MSYS. So whe must explicitly say what type of generator cmake should use.

(Fix issue #83).

* Makefile: Conditionally use "MSYS Makefiles generator".  Also
  refactor the definition of build type by defining an
  CMAKE_CMD_LINE_OPTIONS variable.

@PierreTechoueyres
Copy link
Contributor Author

@tarsius ping ?

@tarsius
Copy link
Member

tarsius commented Nov 21, 2019

I would still prefer it if @TheBB reviewed this.

Could the commit message below be what you expect ?

Mostly. It also needs to begin with a fairly short oneline summary.

@PierreTechoueyres
Copy link
Contributor Author

what about:

Avoid using Visual Studio generator when building on Microsoft Windows.

Prrefer using MSYS Makefiles generator over the one provided by 
Microsoft Visual Studio (Fix issue #83).

In Microsoft Windows, if Visual Studio is installed, it's the preferred choice of
 cmake before MSYS. But Emacs (and this lib by extension) must be build with
 MSYS. So whe must explicitly say what type of generator cmake should use.

* Makefile: Conditionally use "MSYS Makefiles generator".  Also
  refactor the definition of build type by defining an
  CMAKE_CMD_LINE_OPTIONS variable.

@tarsius
Copy link
Member

tarsius commented Nov 21, 2019

I usually don't review commit messages to such an extend, but you asked...

The indentation of the middle paragraph is weird. There are some typos as well; prrefer and whe at least.

Also I don't think you should repeat the same information several times like an American newspaper. In other words drop the last paragraph. Now I realize that I was confused about that variable, so it might make sense to mention it, but the way you are mentioning it right now it doesn't actually do so and so that paragraph doesn't really do much good.

It would be better if you addressed the thing that caused my confusion head on.

CMAKE_CMD_LINE_OPTIONS=-DCMAKE_BUILD_TYPE=Debug

Two variables are mentioned. I am still guessing that CMAKE_BUILD_TYPE is an official variable. If you come to that conclusion, then it is not a stretch to assume that CMAKE_CMD_LINE_OPTIONS is as well. This ambiguity could be addressed by renaming the variable to not use an official sounding name, e.g. BUILD_OPTIONS. (Also why ...CMD_LINE...? That's weird.)

@TheBB
Copy link
Collaborator

TheBB commented Nov 21, 2019

I'm afraid I'm not really in possession of a Windows build environment other than Appveyor, so I'm happy to let those who actually do build on Windows make the decisions they feel they need.

Since @tarsius left some comments I'll wait until tomorrow to merge/adjust.

@PierreTechoueyres
Copy link
Contributor Author

I usually don't review commit messages to such an extend, but you asked...

And I really appreciate. I know I'm not very fluent in English, so every help is welcome.

The indentation of the middle paragraph is weird. There are some typos as well; prrefer and whe at least.

Also I don't think you should repeat the same information several times like an American newspaper. In other words drop the last paragraph. Now I realize that I was confused about that variable, so it might make sense to mention it, but the way you are mentioning it right now it doesn't actually do so and so that paragraph doesn't really do much good.

It would be better if you addressed the thing that caused my confusion head on.

CMAKE_CMD_LINE_OPTIONS=-DCMAKE_BUILD_TYPE=Debug

Two variables are mentioned. I am still guessing that CMAKE_BUILD_TYPE is an official variable. If you come to that conclusion, then it is not a stretch to assume that CMAKE_CMD_LINE_OPTIONS is as well. This ambiguity could be addressed by renaming the variable to not use an official sounding name, e.g. BUILD_OPTIONS. (Also why ...CMD_LINE...? That's weird.)

I'll try to provide something tomorrow or on the following days (now it's too late for me. I really should be sleeping from a long time).

On MS Windows platform Emacs and this lib can not be build with Visual Studio,
so use MSYS Makefiles generator against the one provided by Microsoft
(Fix issue #83).

* Makefile: Conditionally use "MSYS Makefiles generator".  Also refactor the
  definition of build type by defining a BUILD_OPTIONS variable.
@PierreTechoueyres
Copy link
Contributor Author

@tarsius:
Hope the new version will match what you're expected.

@TheBB TheBB merged commit 65fde63 into emacsorphanage:master Dec 17, 2019
@PierreTechoueyres PierreTechoueyres deleted the pte/fix-windows-build branch December 20, 2019 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants