nanoant / CMakePCHCompiler

CMake precompiled header support via custom PCH compiler extension
MIT License
103 stars 20 forks source link

Rebase mick-p1982's PR #13 #19

Closed nanoant closed 6 years ago

nanoant commented 6 years ago

This is rebase of @mick-p1982 PR #13, so it can be merged into the master.

nanoant commented 6 years ago

@mick-p1982 I have reviewed your changed more thoroughly this time. While first "quoting", PRIVATE commits seems to be obvious and I am ready to merge them, the latter ones attempting to solve MSVC PCH removal, and your final last commit's solution of /Z7 looks a bit drastic (and limiting) to me, as long as I understand correctly that this effectively turns of PDB generation in debug builds when using PCH with this module. I wish to look into the issue, can you provide me more information about this? Where & how this was exactly reported to Microsoft if it is unintended behavior.

m-7761 commented 6 years ago

I want the module to be useful, or would just assume the changes not be implemented. There is a long discussion on the Kitware about the z7 and other issues, that I don't want to think about further. It's a miracle that it works at all. The only thing that matters is if you can build with PCH or not. Nothing else matters. (P.S. Before changing, test it with Visual Studio, it won't work otherwise.)

m-7761 commented 6 years ago

@nanoant The short answer is, with PDB Visual Studio will build the PCH target, and then delete its output, and then fail to build the main target. Obviously, that's not viable and never will be.

If the final file looks different from the one I attached, I think it's wise to test it thoroughly, since any small change could break it. Someone should test it if it's different.

nanoant commented 6 years ago

@mick-p1982 I have cleaned up your changes and squashed & split into atomic (IMHO) commits and I am ready to merge them. (See commits above) Right now, all I need is your review of these changes -- both from the source code content and and commit message wording point of view. Also I would appreciate if you can test them, you change check-out this PR into your local repo as described here: https://help.github.com/articles/checking-out-pull-requests-locally/

@gjasny Can you also please have a look at this PR. I do appreciate your input, especially right now on vacation I am really limited in terms of the testing machine access. Thank you in advance.

m-7761 commented 6 years ago

@gjasny mentioned doing tests. I will definitely make an effort to try out the main file here the next time I work with this CMake module (I mainly use it personally for testing work with GCC prior to publishing.) I don't think trying to force an artificial test is good for me. And it's not convenient to do a quick test.

FYI: My feeling about PDB is that it's probably not worth maintaining as an option, because it simply doesn't work with Visual Studio if you use the normal ways of building programs with it. So it would only be useful for publishing the PDB files. And I think that if you are that invested into PDB then it probably makes sense to just use Visual Studio to do that. (I think CMake is for publishing to the needs of unknown users. It's a compromise in other words. So the purpose of this module is mainly so you can publish sources that end-users can build/see for themselves quickly. Secondary to that is having a basic development environment out of the box. But that's not a substitute for a proper development environment... which you'll want if you're spending a significant amount of time with a project.)

m-7761 commented 6 years ago

@nanoant, until there is a live file, I don't think I can locate a copy of the final file via GitHub, so if you can attach one, I can do a diff with my working-copy. Otherwise I can't do a real review.

My main advise is to just don't leave anything out, because everything that is changed is changed for a very good reason, i.e. things don't work otherwise. I assume the final is more or less what I submitted, or what is sitting in my fork (https://github.com/mick-p1982/CMakePCHCompiler)

nanoant commented 6 years ago

@mick-p1982 @gjasny I have tested it with CMake 3.0.2 and Apple Clang, it seems to work fine. Merging. Thanks!

m-7761 commented 6 years ago

Glad to see the public file is updated. With some creativity I checked the RAW button, and copy-pasted it into a new file, for the purpose of diff'ing it with my working copy. (Of course GitHub doesn't provide a download button, why would it?)

Here are some missing lines, but I did not add these, so I assume you (@nanoant) know what these do, and that they are not required... Strikeout: Woops, I was reading the diff backward in these instances. Sorry. (Code removed.)

Looks good to me. I hope it was not too much work. I apologize for not being of more help. My ability with Git is limited to what words to say instead of Checkout/Commit ... and I'm not even sure about the latter! I wish that poking around in confounding mazes of consoles/boxes/buttons were as effortless as talking :)

nanoant commented 6 years ago

@mick-p1982 No worries, I am happy we have made it through.

As for Git, I can really recommend you reading O'Reilly's Version Control with Git, 2nd Edition. I it is really thorough and not only describes how to use Git, but precisely how it works.