[SERVER-48203] Support --install-action for Ninja builds Created: 13/May/20 Updated: 29/Oct/23 Resolved: 24/Aug/22 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Build |
| Affects Version/s: | None |
| Fix Version/s: | 6.1.1, 4.4.18, 5.0.14, 6.0.3, 6.2.0-rc0 |
| Type: | New Feature | Priority: | Major - P3 |
| Reporter: | Ryan Egesdahl (Inactive) | Assignee: | Daniel Moody |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||||||||||||||||||||||
| Backport Requested: |
v6.1, v6.0, v5.0, v4.4
|
||||||||||||||||||||||||||||||||||||
| Sprint: | Dev Platform 2021-01-25, Dev Platform 2021-02-08, Dev Platform 2021-02-22, Dev Platform 2021-03-08, Dev Platform 2021-03-22, Dev Platform 2021-04-05, Dev Platform 2021-04-19, Dev Platform 2021-05-03 | ||||||||||||||||||||||||||||||||||||
| Participants: | |||||||||||||||||||||||||||||||||||||
| Description |
|
We recently added a workaround for compile_ninja so it sets install-action=default to work around the fact that non-default actions do not work with Ninja. The reason: Ninja has no idea what an install-action even is because that's up in the SCons layer. This workaround is mildly hackish, and it would be a better idea to fix the Ninja generator to implement the SCons install-action. |
| Comments |
| Comment by Githook User [ 05/Oct/22 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Daniel Moody', 'email': 'daniel.moody@mongodb.com', 'username': 'dmoody256'}Message: (cherry picked from commit 0054d395a55c60470f5747647a8a7988530d1a5f) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 30/Sep/22 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Daniel Moody', 'email': 'daniel.moody@mongodb.com', 'username': 'dmoody256'}Message: (cherry picked from commit 0054d395a55c60470f5747647a8a7988530d1a5f) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 30/Sep/22 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Daniel Moody', 'email': 'daniel.moody@mongodb.com', 'username': 'dmoody256'}Message: (cherry picked from commit 0054d395a55c60470f5747647a8a7988530d1a5f) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 30/Sep/22 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Daniel Moody', 'email': 'daniel.moody@mongodb.com', 'username': 'dmoody256'}Message: (cherry picked from commit 0054d395a55c60470f5747647a8a7988530d1a5f) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Alex Neben [ 20/Sep/22 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Will you also need to include the changes in https://github.com/10gen/mongo/pull/7213 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 24/Aug/22 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Daniel Moody', 'email': 'daniel.moody@mongodb.com', 'username': 'dmoody256'}Message: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Daniel Moody [ 17/Aug/22 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Wanted to update this in that recently ninja was updated and fix the some underlying issues preventing hardlinks from working correctly. It should have been fixed from this PR: https://github.com/ninja-build/ninja/pull/1943 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Daniel Moody [ 08/Apr/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Created an issue requesting ninja to delete files before building: https://github.com/ninja-build/ninja/issues/1947 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Daniel Moody [ 30/Mar/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Created an issue that can fix the hardlinking problems we were having with ninja: https://github.com/ninja-build/ninja/issues/1940
The code in review still works with this issue present, but results in unnecessary rebuilds after everything is first built. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Andrew Morrow (Inactive) [ 08/Mar/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I too considered the idea of splitting the task into a removal part (I wouldn't necessarily call it 'clean' - as that term sort of means something else) and an execution part. But that seemed like strictly more work than changing ninja -f {} -t compdb {}CC CXX into ninja -f {} -t compdb {}CC_PRECIOUS CXX_PRECIOUS, which I think would have the desired effect? Anyway, I left some comments on the code review and hopefully we can get a new version of this out soon. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Ryan Egesdahl (Inactive) [ 08/Mar/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
daniel.moody If you mean having a clean rule per target, maybe that would work. It might make the build.ninja a bit large, but it would be more in line with the way most other build systems work. In any case, I think that's a better option than trying to use regexes. That seems fragile. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Daniel Moody [ 08/Mar/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
acm We can pull out the recursion, actually when I thought about it more, that is not only risky but also incorrect. For example let a build target which creates a directory and a build target which build something in that directory. In this case, if the directory building node ran it would delete the other target even though one invoking the build may have just requested to rerun the directory building target.
Also for the compile_commands.json a couple of options I first thought of:
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 08/Mar/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Andrew Morrow', 'email': 'acm@mongodb.com', 'username': 'acmorrow'}Message: Revert " This reverts commit 7739da6997795c20924580087a0e09db0a8cf929. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Andrew Morrow (Inactive) [ 08/Mar/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
daniel.moody - The -rf was sufficiently risky given that it is appearing in the compilation database that I've reverted the commit for now. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Andrew Morrow (Inactive) [ 07/Mar/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
billy.donahue - A few things:
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Billy Donahue [ 07/Mar/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Reopening as I think this "mildly hackish" commit is perhaps too hackish and may have to be reconsidered. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Billy Donahue [ 07/Mar/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I'm not sure this is a successful idea. It's breaking clangd. After this commit, I see rm -rf; ... in all my compile_commands.json command elements before the clang++ command. I see this was added to site_scons/site_tools/next/ninja.py, but that's the scariest command in all of Unix. Why is that happening? I don't know why an `rm -rf` is necessary to recompile a .o file. Can't the compiler rewrite a .o without this step? I get, for example,
Anyway this is causing a major problem for me, as clangd takes that "command" string and it thinks my compiler is rm -rf which is scaring the heck out of me. Here's what my clangd stderr (set to verbose logging) shows when I open mutex.cpp:
Ok I think so far that was intentional.
It looks like clangd is examining what it considers to be the compiler argument list, and making decisions based on that, and it sees the rm -rf as a compiler taking a weird compiler flag -rf and it pukes. It never sees the real compiler flags, so it can't find any headers or anything else. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 05/Mar/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Daniel Moody', 'email': 'daniel.moody@mongodb.com', 'username': 'dmoody256'}Message: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Mathias Stearn [ 03/Feb/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
My comment was in reply to ryan.egesdahl's above, explaining why this ticket doesn't actually cover the behavior requested in | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Andrew Morrow (Inactive) [ 03/Feb/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
redbeard0531 - The point of this ticket is to make the configurability that we have at the SCons level apply at the Ninja level as well. Therefore, it does support what you want because you will be able to inform Ninja of whether you want copies, hardlinks, reflinks, install, or anything else. I think everyone understands the difference between a hardlink and a reflink. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Mathias Stearn [ 03/Feb/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
This is actually different from | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Eric Milkie [ 14/Sep/20 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Understood - thank you! | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Ryan Egesdahl (Inactive) [ 14/Sep/20 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
milkie The work in | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Eric Milkie [ 13/Sep/20 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I see that |