[WIP] Install() and InstallAs() migrated to using a command generator as default

[bdbaddog]

Continue the work from PR #3535

Benchmark vs original logic

Using MongoDB's install-all-meta which does a lot of real world install calls (insert # here)

Win32

on NVME SSD

Run	Plain	New
1	135.84	128.28
2	128.42	125.37
3	137.13	129.19
avg	133.80	127.62

on HD

Run	Plain	New
1	491.97	311.75
2	332.30	329.92
3	308.67	319.54
avg	377.65	320.40

Contributor Checklist:

• I have created a new test or updated the unit tests to cover the new/changed functionality.
• I have updated master/src/CHANGES.txt directory (and read the README.txt in that directory)
• I have updated the appropriate documentation

[bdbaddog]

Tagging @chasinglogic as it was originally his PR.

[chasinglogic]

I'm not sure if the / is necessary? It seems like it could lead to some weird situations when there are more than one source.

env.Install(
    target="public",
    source=[
          "web/html",
          "web/js",
          "web/css",
    ]
)

Would expand to something like cp --recursive web/js web/html web/css/ public? Which would still work but it would also work without this weird extra slash.

It's possible depending on how you want this to work you may need to specify $TARGET/ however.

[bdbaddog]

I had /. instead of /'s in my local changes. I just pushed them..
(As advised by: https://unix.stackexchange.com/questions/412259/how-can-i-copy-a-directory-and-rename-it-in-the-same-command )

The current /.'s allow install and installAs to work as they did previously.
And with your example above it works as expected:

scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
cp --preserve=all --recursive web/css/. public/css
cp --preserve=all --recursive web/html/. public/html
cp --preserve=all --recursive web/js/. public/js
scons: done building targets.

$ tree web/
web/
├── css
│   ├── file1
│   ├── file2
│   └── subdir
│       └── file3
├── html
│   ├── file1
│   ├── file2
│   └── subdir
│       └── file3
└── js
    ├── file1
    ├── file2
    └── subdir
        └── file3

$ tree public/
public/
├── css
│   ├── file1
│   ├── file2
│   └── subdir
│       └── file3
├── html
│   ├── file1
│   ├── file2
│   └── subdir
│       └── file3
└── js
    ├── file1
    ├── file2
    └── subdir
        └── file3

[chasinglogic]

LGTM

[chasinglogic]

Should there be an explanation of why these flags were chosen? (i.e. to preserve ownership and permission modes)

[bdbaddog]

Sounds good. Adding.

[chasinglogic]

One other thought (probably outside the scope of these changes) we've been talking about how to do privileged installations similar to a sudo make install without running SCons as root as that would obviously break the sconsign file.

We considered creating a new PrivilegedInstall builder that would escalate itself. But it seems like after these changes you could make the Install builder take a PRIVILEGED=True which would prepend sudo (runas on Windows) to the appropriate INSTALL*COPY command.

Alternatively it could be left as an exercise for the user to use an OverrideEnvironment like so:

env.Install(
    target='/usr/bin',
    source=[mongod],
    INSTALLFILECOPY='sudo $INSTALLFILECOPY',
)

But that's obviously slightly less ergonomic (it doesn't even handle Windows) and requires more intimate knowledge of the implementation for the Install builder.

[bdbaddog]

@chasinglogic - yup I think privileged install is outside the scope. But that said with these changes users can now set the appropriate variables and it's possible.

[mwichmann]

This is really a philosophy thing: should a build tool ever do priviliged stuff? I'd personally hope - never.

[bdbaddog]

@chasinglogic - I can't get the windows version of this to work. When it calls xcopy to copy a single file it's always prompting:

PS C:\Users\Bill\AppData\Local\Temp\testcmd.38820.uc9c3rlk> Xcopy /Q /O /X  .\test1.c test1_c
Does test1_c specify a file name
or directory name on the target
(F = file, D = directory)? F
Access denied
0 File(s) copied

Did you run into this?
If I switch to use copy instead of xcopy for copying files that seems to work.. But not sure about copying permissions...

[chasinglogic]

Hey @bdbaddog sorry for the slow turn around was moving to another country :D

It seems like you figured it out already

[acmorrow]

Per discussion with @bdbaddog, I think having a separate overridable copy operation for directories isn't something that should be included. Imagine I want my install to be a tree of symlinks. If I override $INSTALLFILECOPY to produce symlinks. To make that work for installing a directory, I would also need to override $INSTALLDIRCOPY to something that would walk the tree and make all the symlinks. Instead, if there is only $INSTALLFILECOPY, and we retain the current behavior of using scons_copytree, but update it to use $INSTALLFILECOPY, then the whole thing just works. My opinion is that it is better to have one configurable thing here, specific to files, and let SCons handle the dirwalking.

[acmorrow]

@bdbaddog - I'm revising my prior statement.

I never had too much time to discuss this change with @chasinglogic, but I think there are three things going on here:

1. The first is a desire to potentially improve build performance when installing files, by making it easy to do things like replace file copy with hardlinks, or cp --reflink. This matters for builds like the MongoDB build which makes heavy use of Install for large static binaries. Copying all that data takes time (and disk space), and making better use of the filesystem to avoid needing to actually copy the data can really be worth it. Currently, to make effective use of the $INSTALL configurable to achieve these aims is painful: you basically need to completely re-implement the SCons copyFunc, copyFuncVersionedLib, and copytree functions to do it right.

2. The second goal, though perhaps a less important one, is to move IO out of python and into external processes, as these are likely to be faster.

3. The third goal is specific to MongoDB's current activities. The Ninja backend that we are working on has a limitation: it cannot emit a representation for arbitrary SCons functions. This isn't too surprising: if you define the Action for a builder to be a python function, the Ninja generator has no way of knowing what might be going on there. So when the Ninja generator encounters such a thing, it simply queues it up to re-invoke SCons to produce. This works pretty well, but it is less than ideal, so we try to limit these cases as much as possible. Unfortunately, Install falls into this case because of installFunc. We really don't want the Ninja generator to need to reinvoke SCons just to copy a file, so instead we implement our own definition of installation at the Ninja layer. Unfortunately, this means that customizing $INSTALL at the SCons layer is not reflected in the generated Ninja file, since the Ninja generator is essentially ignoring whatever SCons is setup to do for Installation. The change proposed here turns the installation subsystem into something that the Ninja layer can "see into", and therefore directly project into its generated build.ninja.

Given that, I think it is important that customization for both file installation and directory installation be provided. If we only provided the customization for file installation, and implemented directory installation via a python function that used the file installation customization, then the Ninja layer would still be unable to see how directories were to be installed.

I'll take a look through this review with an eye towards whether the current state meets these needs and think about what I would like the final shape to look like.

[chasinglogic]

@acmorrow that's summed up perfectly

[acmorrow]

Much of the difficulty with this review is that the code reformatting has made it look much more complex than it really is.

[bdbaddog]

@acmorrow - thoughts?
Or should I just close this PR?

[chasinglogic]

FWIW this LGTM. It accomplishes almost all of the goals the original work set out to.

The only outlier is the installVersionedLib which I seem to remember there being a good reason to leave it as a Python func (and really isn't important for Ninja anyway).

There's lots of commented out code that should be cleaned up IMO (didn't comment on all of it). But this looks like it should be merged to me.

[chasinglogic]

Should probably just delete this?