General improvements to Linux story #23507

khyperia · 2017-11-30T23:33:05Z

Delete files that are no longer being used
- build/linux folder
- build/unix folder
- build/scripts/unzip.sh
auto-install dotnet if missing or wrong version (allows user to walk up and ./build.sh --restore --build --test on a clean machine, or a machine with 2.0.x installed globally)
move cibuild.sh to build/scripts/cibuild.sh (to match cibuild.ps1), and significantly simplify it
add shorthands to --restore, --build, and --test
fix bug where build_configuration wasn't propogated to tests.sh

Let me know if this should target features/compiler instead of master

khyperia · 2017-12-04T16:23:45Z

@jaredpar Something I'd specifically like feedback on is the "auto-install dotnet" point. Automatically doing stuff seems iffy - I can put it behind a flag (--install-dotnet? But that's not really a good name). There's a lot of use cases here, and balancing them all is difficult:

New contributor walking up to the repo and trying to build. This should be as simple as possible, and hopefully using minimal flags.
Existing developer doing an inner dev loop - fine control perhaps should be allowed (e.g. not auto-installing dotnet if missing/wrong version, for whatever reason)
CI build - verbosity of parameters here isn't a problem, but perhaps not to the point of providing a --ci flag that subtly changes behavior of things.

Actually, one helpful point that could help the "new contributor" case is when no arguments are given (just ./build.sh), either error with a message giving a how-to-build command ("Error blah no args blah: build with ./build.sh --install-dotnet --restore --build --test), or default to sane arguments and keep going (perhaps printing the "virtual arguments" used).

Edit: Oh, I could also gate it inside the --restore flag. That seems pretty reasonable...

Edit 2: Also important to note is that ./obtain_dotnet.sh doesn't necessarily install the CLI. If dotnet is already in the PATH, AND is the correct version (the one specified in tools.props), then we just return immediately.

jaredpar · 2017-12-04T17:08:38Z

New contributor walking up to the repo and trying to build. This should be as simple as possible, and hopefully using minimal flags

This is the experience I give the most weight too. I should be able to go up to any repo clone it and build with 0 hassle.

Something I'd specifically like feedback on is the "auto-install dotnet" point. Automatically doing stuff seems iffy

The experience you're advocating here is essentially the experience we have on Windows today. I had similar misgivings when I set that experience up but so far the feedback's been overwhelmingly positive. Until we get strong feedback otherwise I think we should continue with the approach of "just make it work".

jaredpar · 2017-12-04T17:09:59Z

build.sh

@@ -55,15 +54,15 @@ do
        build_configuration=Release
        shift 1
        ;;
-        --restore)
+        --restore|-r)


Did you consider allowing -restore as well? That would mean there was at least one argument form that worked across windows and linux (build.cmd and build.sh respectively). Alternatively we could try and make the -- syntax work in Windows as well.

Yeah 🙁. This is a very ugh-inducing catch-22. The very, very standard Linux (unix?) convention is that single-dash arguments can be combined (something that I should implement here) - for example, ./build.sh -rbt == ./build.sh -r -b -t). So theoretically, ./build.sh -restore is equivalent to ./build.sh -r -e -s -t -o -r -e, which is... not what one wants. However, the Windows convention here is single-dash, which as you say, hopefully should be identical across platforms... 😢

The best thing would probably be to just say screw linux convention and make -arg==--arg, which makes me a sad panda, but oh well.

The best thing would probably be to just say screw linux convention and make -arg==--arg, which makes me a sad panda, but oh well.

Hmm. Lets keep it as is for now. We can revisit later when we discuss other unification aspects.

jaredpar · 2017-12-04T17:11:05Z

build.sh

+if [[ "${stop_vbcscompiler}" == true ]]
+then
+    echo "Stopping VBCSCompiler"
+    dotnet "${bootstrap_path}"/bincore/VBCSCompiler.dll -shutdown


Shouldn't the path being used here be different depending on whether ${use_bootstrap} is set?

Yeah... although finding VBCSCompiler.dll is pretty difficult when ${use_bootstrap} is false. Because stop_vbcscompiler is a pretty niche feature, I'm tempted to just make it be an error to (${use_bootstrap} == false && ${stop_vbcscompiler} == true)

Hmm. Hadn't thought about that. It actually needs to be first class supported because it's critical to having a sane infrastructure. All the cases we stop the server in our infra are cases where others may need to as well.

Lets work with the CLI team separately from this PR to figure that out.

It's moments like these where I really wish the architecture of the server was per-msbuild-invocation and had a strict parent/child relationship and msbuild was responsible for cleaning up the server... oh well, I can see the argument for the perf bonus on multiple repeated invocations on small solutions.

jaredpar · 2017-12-04T17:11:45Z

build/linux/setup-certs.sh

-mozroots --import --machine --sync
-echo "Y" | certmgr -ssl https://go.microsoft.com
-echo "Y" | certmgr -ssl https://nugetgallery.blob.core.windows.net
-echo "Y" | certmgr -ssl https://nuget.org


Oh wow ... this takes me back. 😄

jaredpar · 2017-12-05T05:00:09Z

build.sh

+        echo "Stopping VBCSCompiler"
+        dotnet "${bootstrap_path}"/bincore/VBCSCompiler.dll -shutdown
+    else
+        echo "--stop-vbcscompiler requires --use-bootstrap. Ignoring and continuing anyway..."


Prefer we exit here. The caller asked for --stop-vbcscompiler and we were unable to fulfill their wishes. We should exit to make sure they're aware of it.

…se-bootstrap

khyperia added the Area-Infrastructure label Nov 30, 2017

khyperia requested a review from a team November 30, 2017 23:33

khyperia mentioned this pull request Dec 1, 2017

CompilerServerUnitTests.MultipleSimultaneousCompiles failed in CI #22913

Closed

khyperia added 3 commits December 4, 2017 08:10

Clean up linux story a bit

04c5b2b

Move cibuild.sh

7ba634a

Move stopping VBCSCompiler to build.sh

0aff565

khyperia force-pushed the linux_stuff branch from f14a8ef to 0aff565 Compare December 4, 2017 16:15

jaredpar reviewed Dec 4, 2017

View reviewed changes

Print error when --stop-vbcscompiler isn't used with --use-bootstrap

3b56448

khyperia force-pushed the linux_stuff branch from 6ca8adf to 3b56448 Compare December 4, 2017 18:22

jaredpar reviewed Dec 5, 2017

View reviewed changes

Abort instead of silently continue on --stop-vbcscompiler without --u…

e19143d

…se-bootstrap

jaredpar approved these changes Dec 6, 2017

View reviewed changes

khyperia closed this Dec 6, 2017

khyperia reopened this Dec 6, 2017

khyperia merged commit 2c1d648 into dotnet:master Dec 7, 2017

khyperia deleted the linux_stuff branch December 7, 2017 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General improvements to Linux story #23507

General improvements to Linux story #23507

khyperia commented Nov 30, 2017

khyperia commented Dec 4, 2017 •

edited

Loading

jaredpar commented Dec 4, 2017

jaredpar Dec 4, 2017

khyperia Dec 4, 2017

jaredpar Dec 4, 2017

jaredpar Dec 4, 2017

khyperia Dec 4, 2017

jaredpar Dec 4, 2017

khyperia Dec 4, 2017

jaredpar Dec 4, 2017

jaredpar Dec 5, 2017

General improvements to Linux story #23507

General improvements to Linux story #23507

Conversation

khyperia commented Nov 30, 2017

khyperia commented Dec 4, 2017 • edited Loading

jaredpar commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khyperia commented Dec 4, 2017 •

edited

Loading