Nanoant pull request

[m-7761]

Hi!

I think I've finally straightened everything out. It's almost impossible to go over everything here. Luckily after so many extensive changes this PR is saying "Able to merge" so I think it's good to just go with it.

Some reservations: I think it would be better if the PCH file went into the binary folders in the MSVC targets, but I don't know how to do that. Currently it goes into the CMakeFiles folders, and it's not release/debug specific, so a clean is probably needed to switch from debug to release, etc. This is nothing new. (Also including headers in the target is not practical. Under GCC it won't work, and I'm unsure about MSVC, but in general, the module cannot consider this use case, and users must accept this limitation. End-users can include the headers themselves I think, manually. But CMake conflates headers with sources, and the custom PCH compilers collide with CMake's naive approach.)

https://github.com/nanoant/CMakePCHCompiler/issues/12 (EDITED: Previously listed as 7. Sorry.)

Because of code in an MSVC script file, called Microsoft.Cpp.Win32.targets, the old way would not normally work on MSVC2010 forward. I worked on many things (paths with spaces, install(EXPORT), and so on) that I can't all recollect before getting to the final step, only to not be able to build under MSVC.

Today I tried some things based on readings, and made it work by forcing /Z7 and including the OBJ outputted by the PCH target in the target sources. This is a straightforward fix compared to using /Zi and PDB and avoids PDB altogether, which was an additional synchronization problem before. Currently only the PCH/GCH files must be referred to by their naked paths. I'd love it if $ worked for all purposes, but I'm pretty sure that it doesn't. I may explore this and file a feature-request with CMake if to no avail.

I've had the opportunity to better understand the module after spending so much time struggling to make it work in a real cross-platform use-case. So I've also made many nuanced improvements along the way, to I think, make it easier to understand. There were some unexplained sly tricks, which I've not added comments on, but have duplicated and changed the names of some variables to better communicate their intent.

https://github.com/nanoant/CMakePCHCompiler/issues/4

Also in this is a fix for the outstanding NOTFOUND issue, which is actually where my work began, before getting sucked down the proverbial rabbit hole. I don't know if this simple fix will solve everyone's NOTFOUND issue, but it solved mine, and I expect it will solve many if not all's.

This MSVC fix preserves the REUSE feature, although I've not tested it. To MSVC the PCH is always technically reused, because it's shared by the project-module that generates it and the consuming module. The way CMakePCHCompiler works (its workflow) is really not consistent with MSVC, but I prefer to code it this way, so that it follows the same beats as the GCH pathway; and in fact, if it diverged into the more natural MSVC approach, the REUSE option might otherwise not have worked, or would not be practical. So this is a happy ending.

In any case, this works for me, and I can't think of anything to change off the top of my head :)

[m-7761]

I see there were lot of trial and error in your commits, while I really respect your work, I'd love to ask you to squash the commits that introduce then revert back some changes, just to keep the history nice and clean. If this is too much I am okay to merge this as is.

No trial and error. Just bite sized commits. It's everything I did to my fork. All significant improvements.

I don't think it requires much thought, except that https://github.com/nanoant/CMakePCHCompiler/pull/9 is also in there. But I probably edited their contribution also. If it's important, you might try to do theirs first, and then see if this one still merges... just for attribution sake? If you like :)

[m-7761]

@nanoant For what it's worth, I noticed yesterday someone Starred my account's fork. That someone found it/found it useful prompted me to return here and make it more easy to find (https://github.com/nanoant/CMakePCHCompiler/issues/15)

[m-7761]

P.S. I would like an attribution in the README text, if not the Copyright form in the source. I would not normally ask this, except I think I did contribute a lot in this case, and that the potential for this little module in terms of how many lives it can touch is not to be underestimated.

Most of the edits are to do escapes correctly, and take care to not clobber/discard user directives. I mostly ran into these problems for myself, but whenever I learned something new (from a bad episode) I was careful to apply the principles I learned to the module's script. It's more resilient as a result. I think the original needed more attention/testing. I think this PR takes it to a near complete status. Especially concerning GCC and Visual Studio.

[nanoant]

@mick-p1982 I don't mind adding you to README etc. I have merged #9, then I tried to rebase your changes, but it does not cleanly. Can you please provide rebased PR taking into consideration following notes:

1. 1st commit 3389828 if(NOT lenght) return() endif() should be sufficient, no need to wrap whole block.
2. I still would like to see the commits fda58e2 b117be4 a26810c 3d32634 squashed into 7386508 as you have anyway noted "Also, undo any effort in the prior commits to enable headers in sources. (It's not happening.)" so no point in including that, also squashing gets rid of WTF in the comment that gets later removed and I anyway don't really want there.
3. Also I don't think there is a point to keep lot of code commented out, otherwise it looks like half-baked and it is hard to get a point of the changes.
4. And finally I humbly ask to mind the formatting, no trailing spaces (see screenshot below - Git really complains about this) so this shouldn't be big deal to spot and finally space after # when doing comments.

   I hope you won't find these as being too picky.

[m-7761]

Is "lenght" really spelled that way? (In if(NOT lenght) return() endif()) I think that "if(0!=length)" doesn't work. if(NOT 0 EQUAL length) is the final comparison. I was learning CMake at the same time. I've now forgotten CMake, so maybe this is unremarkable.

I apologize, I'm not well-versed with Git/GitHub. If you can do inline edits (as an owner) I'd appreciate it. I'm not sure how much help I can be. I will try to include my working copy in this comment somehow... either inline or attached. (I apologize, it would only accept a ZIP... or not a cmake file.)

I have one uncommitted change removing "pdb_dir" like so:

# ensure pdb goes to the same location, otherwise we get C2859
            get_filename_component(
                pdb_dir
                "${CMAKE_CURRENT_BINARY_DIR}${CMAKE_FILES_DIRECTORY}/${target}.dir"
                ABSOLUTE
                )

This is just an unused variable.

Your observations are fine. Use common sense by all means. I am not sure about "WTF" I don't see it in my working copy.

White space is non functional, so not worth a moments thought to me. Sorry.

I don't see any commented-out code in my working copy. I think these things will be revised away.

P.S. I don't care if the PR is finalized, as long as you can update the code. We can agree it's closed, OK?

CMakePCHCompiler.zip

[nanoant]

Is "lenght" really spelled that way? (In if(NOT lenght) return() endif()) I think that "if(0!=length)" doesn't work.

No it isn't. That was a typo. I am on vacation just typing in the evening at the terrace, poor light conditions, etc. 🙄

if(NOT 0 EQUAL length) is the final comparison. I was learning CMake at the same time. I've now forgotten CMake, so maybe this is unremarkable.

Sure. No problem. I'll redact it, maybe I'll make then a branch for your review before I merge it into master. What do you think?

I apologize, I'm not well-versed with Git/GitHub. If you can do inline edits (as an owner) I'd appreciate it. I'm not sure how much help I can be. I will try to include my working copy in this comment somehow... either inline or attached. (I apologize, it would only accept a ZIP... or not a cmake file.)

No worries.

I have one uncommitted change removing "pdb_dir" like so: (...)

Okay, I'll try to do my best to get these changes consolidated. The only problem is that I have no access to Windows machine at the moment, so it may be tricky to test these changes.

Your observations are fine. Use common sense by all means. I am not sure about "WTF" I don't see it in my working copy.

See https://github.com/nanoant/CMakePCHCompiler/pull/13/commits/7386508d248fff29b4ef618d712359594a33139c

White space is non functional, so not worth a moments thought to me. Sorry.

It does appear as a change at least in Git, even if non-functional produces unnecessary history noise.

I don't see any commented-out code in my working copy. I think these things will be revised away.

See https://github.com/nanoant/CMakePCHCompiler/pull/13/commits/62e2707b98edb1b2e72e7fb0fe5209813ca0179c e.g. #set(debug_flags " /Z7")

P.S. I don't care if the PR is finalized, as long as you can update the code. We can agree it's closed, OK?

Sure. Like I've said above, I'll try to rebase it and I'll put it into a branch for your final review. Just give me a bit more time.

[m-7761]

Just for the record, my feeling (in general) is I already put many unanticipated hours into this, just out of necessity. By requesting its integration, I am wanting to make it more easily available, but if it means I have to think about it (or type anything) for more than a few minutes, I have to bow out :)

I have an idea about how long it would take me to assess these changes and commit them to an SVN repository. That is about 2 min. So I cannot justify giving more than 2min of my time to this PR. I apologize, and hope that isn't an obstacle to the inclusion of this work. My experience is just that if I am not in control of the account, I don't want to be whipped around at its mercy. So I think @nanoant just has to make the calls here, and if that means forgoing this PR, that's their decision. I cannot be involved beyond donating this code. I don't have the time or patience to get involved and to be honest the micromanagement of open source code always seems neurotic and humiliating to me.... my feeling is make the change in 1min, because version systems exist so you can repair any problem that comes up in the future. Peace.

P.S. It works on Windows. If you want to take time out of your schedule to see for yourself by all means. CMakePCHCompiler doesn't work as-is currently. I use it on a regular basis with many version of Visual Studio and GCC on several systems. The reason this PR exists is because I had to make it work. I use it for extremely heavy loads; programs that would take days to build without it. It works both in the technical sense and under high stress, in complex multi PCH build settings.

[gjasny]

@mick-p1982 thank you for working on that topic. I gave that branch a try on our giant code base and have some more fixes for ninja generator to push. But I'd like to see this PR merged first.

Maybe I find time to do the requested changes.

[m-7761]

@gjasny I think all developers would be doing themselves a favor to adopt this module until CMake delivers a viable alternative in the form of a first-class feature. And even then, this module is invaluable for back compatibility. When I think back on my time as a developer, I can't imagine what it would have been like without PCH. I think it would be good to spread it far and wide.

There are many things developers can do to increase their productivity many times over that just boils down faster build-test loops by any means necessary. Ninja is nice for shaving off microseconds in linear time. PCH is good if you want to save yourself an entire weekend in nonlinear time!

(CMake's policy on this makes about as much sense as, well, many things in our world that seem to make no sense. Sorry, I try not to dwell on our species' penchant for senseless behavior.)

[nanoant]

@mick-p1982 I hope I can correctly assume I am the recipient of your post, hence please see my response below your text.

Just for the record, my feeling (in general) is I already put many unanticipated hours into this, just out of necessity. By requesting its integration, I am wanting to make it more easily available, but if it means I have to think about it (or type anything) for more than a few minutes, I have to bow out :)

First of all, neither I want to discourage you, nor I have anything against your changes and merging them into my repo. The reason it takes so long from your perspective is that:

1. I don't believe this is right way to go. In my opinion the way to go is the native implementation. Therefore I was reluctant to spend any time longer on this proof-of-concept module. The only reason I somehow changed mind I the recent statement of KitWare's @bradking that they no interest implementing PCH, regardless it is makes a great deal for some CMake's users. (as stated at https://gitlab.kitware.com/cmake/cmake/issues/1260#note_428205) I don't want to judge them, they have full right to decide about CMake, I am maybe just disappointed that we did not move forward with that since 2014.

2. I no longer work on any project based on CMake. Few years ago CMake was the best way to go, nowadays there're couple of better alternatives.

3. Finally, I try to make all the source code I release or supervise under my account follow some well established standards in terms of wording, readability, organization, etc. Hence my requests about rebase & squash, trailing spaces and cleaning up some comments. In my opinion these are nothing fancy. Note: I have submitted few patches to CMake itself, it wasn't easy process, and KitWare was far more picky about variable names, code layout, etc. I was not complaining, because many (or rather most) well known projects follow strict standards -- again this is nothing unusual.

I have an idea about how long it would take me to assess these changes and commit them to an SVN repository. That is about 2 min. So I cannot justify giving more than 2min of my time to this PR.

Not sure who would spend 2 min on reviewing 11 commit PR. Whoever that was I would not definitely work with that guy 😛 Again (you may believe me or not) but I spend far more time (without any compensation) on numerous open-source projects polishing my PRs so they get past the review positively. The prize was that I have put my little brick to some app that use or have been using and it is working now as expected.

I apologize, and hope that isn't an obstacle to the inclusion of this work. My experience is just that if I am not in control of the account, I don't want to be whipped around at its mercy. So I think @nanoant just has to make the calls here, and if that means forgoing this PR, that's their decision. I cannot be involved beyond donating this code. I don't have the time or patience to get involved and to be honest the micromanagement of open source code always seems neurotic and humiliating to me.... my feeling is make the change in 1min, because version systems exist so you can repair any problem that comes up in the future. Peace.

Ouch, this is getting way emotional. I have said already that if you don't want to make the requested changes I will do them myself and let you review them as a branch prior committing them to master -- simply because I want you to remain the author, because I respect your contribution.

P.S. It works on Windows. If you want to take time out of your schedule to see for yourself by all means. CMakePCHCompiler doesn't work as-is currently. I use it on a regular basis with many version of Visual Studio and GCC on several systems. The reason this PR exists is because I had to make it work. I use it for extremely heavy loads; programs that would take days to build without it. It works both in the technical sense and under high stress, in complex multi PCH build settings.

I believe you, therefore I don't think I need to do any further tests. Just give me 1-2 days to get the branch ready, as I said before I am on vacation and I am seriously limited (because of family reasons). 🤓

[nanoant]

@mick-p1982 @gjasny I have rebased this PR into new #19. Let's continue the review there.

[m-7761]

@nanoant Believe me, I'm not "emotional" in the slightest. And as far as waiting goes, it's in your hands.... this PR has been here for months after all. Hell, even a year. It's not dated July 2018... it's from 2017!

[m-7761]

EDITED: I noticed I cited Issue 7 in the original post/text. I think I meant #12. I apologize for confusion. I probably had both open in tabs at the time :(