[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:
Backports
Duplicate
is duplicated by SERVER-25534 Use "cp --reflink=auto" (or similar) ... Closed
Problem/Incident
causes SERVER-69828 Ninja hardlink changes causing issues... Closed
Related
related to SERVER-55021 Complete TODO listed in SERVER-48203 Closed
related to SERVER-69133 remove redundant setting of hardlink ... Closed
related to SERVER-54232 Use install rather than cp to copy fi... Closed
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: SERVER-48203 add precious and link-type install actions to ninja

(cherry picked from commit 0054d395a55c60470f5747647a8a7988530d1a5f)
Branch: v6.1
https://github.com/mongodb/mongo/commit/0c6875e3b3daa7adf8ae75fd68b2002c1409bb43

Comment by Githook User [ 30/Sep/22 ]

Author:

{'name': 'Daniel Moody', 'email': 'daniel.moody@mongodb.com', 'username': 'dmoody256'}

Message: SERVER-48203 add precious and link-type install actions to ninja

(cherry picked from commit 0054d395a55c60470f5747647a8a7988530d1a5f)
Branch: v5.0
https://github.com/mongodb/mongo/commit/59ee2c548e05fdbf143ed061d4381064007f5bcd

Comment by Githook User [ 30/Sep/22 ]

Author:

{'name': 'Daniel Moody', 'email': 'daniel.moody@mongodb.com', 'username': 'dmoody256'}

Message: SERVER-48203 add precious and link-type install actions to ninja

(cherry picked from commit 0054d395a55c60470f5747647a8a7988530d1a5f)
Branch: v6.0
https://github.com/mongodb/mongo/commit/80d49ccf6a90e5d7791f071c93d489e00b5fddc0

Comment by Githook User [ 30/Sep/22 ]

Author:

{'name': 'Daniel Moody', 'email': 'daniel.moody@mongodb.com', 'username': 'dmoody256'}

Message: SERVER-48203 add precious and link-type install actions to ninja

(cherry picked from commit 0054d395a55c60470f5747647a8a7988530d1a5f)
(cherry picked from commit 235038c6d57716e3ecdb9f15726a78689c80dd07)
Branch: v4.4
https://github.com/mongodb/mongo/commit/e78dbe46f3981a98acbfd34c19536b3e65769423

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: SERVER-48203 add precious and link-type install actions to ninja
Branch: master
https://github.com/mongodb/mongo/commit/0054d395a55c60470f5747647a8a7988530d1a5f

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:

  • We have a compiledb target which creates the compile commands json, so we could regex out the delete commands after the json is created by ninja.
  • We could not touch the original build commands in ninja, and instead create a CLEAN rule which we then set as a dependency to the normal non precious build nodes.
Comment by Githook User [ 08/Mar/21 ]

Author:

{'name': 'Andrew Morrow', 'email': 'acm@mongodb.com', 'username': 'acmorrow'}

Message: Revert "SERVER-48203 setup ninja install actions rule, make evergreen fail correctly on ninja, add precious handling"

This reverts commit 7739da6997795c20924580087a0e09db0a8cf929.
Branch: master
https://github.com/mongodb/mongo/commit/f30dbae001e83286c6aa18d566d1628d335b9d74

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:

  • While we look into this, you can back down to `--build-tools=stable` (the default, so you can just omit --build-tools=next) and you won't get this behavior. Unless there is a specific thing you need from --build-tools=next.
  • I had thought that we dialed back from rm -rf but it appears not. daniel.moody, I think we ought to at least change that to be rm -f: the recursion is indeed scary, and I think not necessary.
  • Regarding why we added this behavior, we are actually more faithfully modeling the SCons behavior here. It always removes target files before writing to them unless marked as Precious. This is much more important in the hardlink case, details are in the (extensive) CR discussion.
  • The compile_commands.json issue is more interesting, and something we clearly overlooked. We will definitely investigate how we can still achieve the desired behavior of removing targets before rebuilding them without affecting compilation database generation.
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,

  {
    "directory": "/Users/billy/prog/mongodb/mongo",
    "command": "rm -rf build/ninja/mongo/platform/mutex.o; export PATH='/usr/local/bin:/opt/bin:/bin:/usr/bin';export PATHOSX='/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin:/Library/Apple/usr/bin';/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ -o build/ninja/mongo/platform/mutex.o -c -Woverloaded-virtual -Werror=unused-result -Wpessimizing-move -Wredundant-move -Wno-undefined-var-template -Wno-instantiation-after-specialization -fsized-deallocation -Wno-defaulted-function-deleted -Wunused-exception-parameter -stdlib=libc++ -std=c++17 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk -mmacosx-version-min=11.0 -target darwin17.0.0 -arch x86_64 -Wno-implicit-int-float-conversion -fno-omit-frame-pointer -fno-strict-aliasing -fasynchronous-unwind-tables -ggdb -Wall -Wsign-compare -Wno-unknown-pragmas -Winvalid-pch -Werror -O2 -Wno-unused-local-typedefs -Wno-unused-function -Wno-unused-private-field -Wno-deprecated-declarations -Wno-tautological-constant-out-of-range-compare -Wno-tautological-constant-compare -Wno-tautological-unsigned-zero-compare -Wno-tautological-unsigned-enum-zero-compare -Wno-unused-const-variable -Wno-missing-braces -Wno-inconsistent-missing-override -Wno-potentially-evaluated-expression -Wno-unused-lambda-capture -Wno-exceptions -Wunguarded-availability -fstack-protector-strong -fno-builtin-memcmp -MMD -MF build/ninja/mongo/platform/mutex.o.d -DSAFEINT_USE_INTRINSICS=0 -DPCRE_STATIC -DNDEBUG -DBOOST_THREAD_VERSION=5 -DBOOST_THREAD_USES_DATETIME -DBOOST_SYSTEM_NO_DEPRECATED -DBOOST_MATH_NO_LONG_DOUBLE_MATH_FUNCTIONS -DBOOST_ENABLE_ASSERT_DEBUG_HANDLER -DBOOST_LOG_NO_SHORTHAND_NAMES -DBOOST_LOG_USE_NATIVE_SYSLOG -DBOOST_LOG_WITHOUT_THREAD_ATTR -DABSL_FORCE_ALIGNED_ACCESS -Isrc/third_party/variant-1.4.0/include -Isrc/third_party/SafeInt -Isrc/third_party/pcre-8.42 -Isrc/third_party/fmt/dist/include -Isrc/third_party/boost-1.70.0 -Isrc/third_party/abseil-cpp-master/abseil-cpp -Ibuild/ninja -Isrc src/mongo/platform/mutex.cpp",
    "file": "src/mongo/platform/mutex.cpp",
    "output": "build/ninja/mongo/platform/mutex.o"
  },

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.
I really don't want my flaky lsp vim plugins running around applying rm -rf to stuff, thinking it's a C++ compiler.

Here's what my clangd stderr (set to verbose logging) shows when I open mutex.cpp:

I[13:57:28.628] Loaded compilation database from /Users/billy/prog/mongodb/mongo
I[13:57:28.640] Enqueueing 4534 commands for indexing
I[13:57:28.645] ASTWorker building file /Users/billy/prog/mongodb/mongo/src/mongo/platform/mutex.cpp version 1 with command
[/Users/billy/prog/mongodb/mongo]
/bin/rm -rf build/ninja/mongo/platform/mutex.o; export PATH=/usr/local/bin:/opt/bin:/bin:/usr/bin;export PATHOSX=/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin:/Library/Apple/usr/bin;/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ -o build/ninja/mongo/platform/mutex.o -c -Woverloaded-virtual -Werror=unused-result -Wpessimizing-move -Wredundant-move -Wno-undefined-var-template -Wno-instantiation-after-specialization -fsized-deallocation -Wno-defaulted-function-deleted -Wunused-exception-parameter -stdlib=libc++ -std=c++17 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk -mmacosx-version-min=11.0 -target darwin17.0.0 -arch x86_64 -Wno-implicit-int-float-conversion -fno-omit-frame-pointer -fno-strict-aliasing -fasynchronous-unwind-tables -ggdb -Wall -Wsign-compare -Wno-unknown-pragmas -Winvalid-pch -Werror -O2 -Wno-unused-local-typedefs -Wno-unused-function -Wno-unused-private-field -Wno-deprecated-declarations -Wno-tautological-constant-out-of-range-compare -Wno-tautological-constant-compare -Wno-tautological-unsigned-zero-compare -Wno-tautological-unsigned-enum-zero-compare -Wno-unused-const-variable -Wno-missing-braces -Wno-inconsistent-missing-override -Wno-potentially-evaluated-expression -Wno-unused-lambda-capture -Wno-exceptions -Wunguarded-availability -fstack-protector-strong -fno-builtin-memcmp -DSAFEINT_USE_INTRINSICS=0 -DPCRE_STATIC -DNDEBUG -DBOOST_THREAD_VERSION=5 -DBOOST_THREAD_USES_DATETIME -DBOOST_SYSTEM_NO_DEPRECATED -DBOOST_MATH_NO_LONG_DOUBLE_MATH_FUNCTIONS -DBOOST_ENABLE_ASSERT_DEBUG_HANDLER -DBOOST_LOG_NO_SHORTHAND_NAMES -DBOOST_LOG_USE_NATIVE_SYSLOG -DBOOST_LOG_WITHOUT_THREAD_ATTR -DABSL_FORCE_ALIGNED_ACCESS -Isrc/third_party/variant-1.4.0/include -Isrc/third_party/SafeInt -Isrc/third_party/pcre-8.42 -Isrc/third_party/fmt/dist/include -Isrc/third_party/boost-1.70.0 -Isrc/third_party/abseil-cpp-master/abseil-cpp -Ibuild/ninja -Isrc src/mongo/platform/mutex.cpp -fsyntax-only -resource-dir=/usr/local/Cellar/llvm/11.1.0/lib/clang/11.1.0
I[13:57:28.985] Loaded compilation database from /Users/billy/prog

Ok I think so far that was intentional.
But then the json output of the clangd connection shows this as the top diagnostic, and there's a cascade of errors from there onward and basically the file can't be indexed.

{
  "jsonrpc": "2.0",
  "method": "textDocument/publishDiagnostics",
  "params": {
    "diagnostics": [
      {
        "code": "drv_unknown_argument",
        "message": "Unknown argument: '-rf'",
        "range": {
          "end": {
            "character": 0,
            "line": 0
          },
          "start": {
            "character": 0,
            "line": 0
          }
        },
        "severity": 1,
        "source": "clang"
      },
      {
        "code": "pp_file_not_found",
        "message": "In included file: 'memory' file not found\n\nsrc/mongo/platform/mutex.h:32:10:\nnote: error occurred here",
        "range": {
          "end": {
            "character": 33,
            "line": 29
          },
          "start": {
            "character": 9,
            "line": 29
          }
        },
        "severity": 1,
        "source": "clang"
      },
      ... more elements follow along those lines...
    ],
    "uri": "file:///Users/billy/prog/mongodb/mongo/src/mongo/platform/mutex.cpp",
    "version": 1
  }
}

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: SERVER-48203 setup ninja install actions rule, make evergreen fail correctly on ninja, add precious handling
Branch: master
https://github.com/mongodb/mongo/commit/7739da6997795c20924580087a0e09db0a8cf929

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 SERVER-25534. That was a request to just Do The Right Thing on filesystems that support reflinks, rather than about adding configurability.

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 SERVER-25534 in the same way that reflink is different from a hardlink. A reflink is semantically an independent copy that just happens to share the underlying blocks using copy-on-write semantics in the file system. A hardlink is semantically shared because changes via one path show up via accesses to the other path. It is always safe to replace a copy with a reflink (if it is supported by the filesystem), but it is risky to replace it with a hardlink.

Comment by Eric Milkie [ 14/Sep/20 ]

Understood - thank you!

Comment by Ryan Egesdahl (Inactive) [ 14/Sep/20 ]

milkie The work in SERVER-25534 is obsoleted by the work in this ticket. We can already do --install-action=hardlink with SCons builds, which is functionally similar to the work described in SERVER-25534, except that it is applicable to all filesystems including btrfs. The work in this ticket involves applying that work to Ninja builds as well.

Comment by Eric Milkie [ 13/Sep/20 ]

I see that SERVER-25534 is marked as a duplicate of this ticket - but is that a mistake? The descriptions of the work for the two tickets are very different, and they are describing two different problems to be solved.

Generated at Thu Feb 08 05:16:26 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.