update mono-devel in dockerfile, and don't cache later commands #32885

chsienki · 2019-01-28T21:41:19Z

This fixes a couple of related Mono CI things:

Pass --debug on the command line for future debugging:
- We invoke xunit via arcade, which has limited extensibility points. I've hooked in a target with a bit of quote foo to enable it. It's not very pretty, I'll open a separate bug on Arcade to let us do this in the future more elegantly.
Docker compose caches layers, causing out of date mono builds:
- We grab mono as part of the docker container build out. If the underlying machines hang around for a while, we'll never rebuild due to caching.
- This PR adds a dummy arg to the dockerfile that we pass in as current datetime; this allows us to cache up to that point, then always rebuild afterwards (see New feature request: Selectively disable caching for specific RUN commands in Dockerfile moby/moby#1996)
- We add a new apt-get update/upgrade pair after the dummy arg, so we'll always update mono to the latest regardless of caching, but its much quicker than always rebuilding from scratch
A mono upgrade bug between 5.21 and 5.23 breaks apt-get update:
- Sadly we hit this bug (Unable to update ca-certificates-mono package mono/mono#12316) upgrading between our cached images and latest
- For now we need to rebuild docker images from scratch in order to flush out the earlier build from all of our build machines
- When all the machines in the pool have cycled through we can remove the --no-cache option and just do an incremental update (bullet 2).

chsienki · 2019-01-29T01:17:43Z

@dotnet/roslyn-infrastructure , @agocke , @jaredpar for review please

jaredpar · 2019-01-29T01:27:40Z

eng/docker/Mono/Dockerfile

+# Update previously installed mono-devel.
+# Cache bust ensures we'll always run the commands following regardless of docker cache status
+ARG CACHE_BUST=0
+RUN apt-get update && apt-get upgrade -y


Should we restrict this to just mono-devel?

Me and @agocke spoke about this yesterday. We figured that we should keep everything up to date as otherwise we could get into weird states where an 'incremental' build fails differently to a new build due to package versioning diffs.

My thinking was to update all, then restrict if we see it taking too long in the future.

Running this in the middle will disable caching for all subsequent statements. Is that what you intended?

Yes, we edit the user permissions to make them match the outer user. That causes the upgrade/update to have issues, so it's simpler to do it before hand. The user stuff takes a trivial amount of time to run, so we aren't hurt by not caching it. (Also, we have no guarantees that the outer user will stay the same, so we'd potentially not want to cache it anyway, although the chance of it changing and the cache remaining seem slim)

In reply to: 251963406 [](ancestors = 251963406)

jaredpar · 2019-01-29T01:28:41Z

eng/targets/XUnit.targets

+        adding our argument in, then having a second empty quoted argument before the real args. Hmmmmmm.
+    -->
+    <PropertyGroup>
+      <MonoTool>$(MonoTool)" --debug"</MonoTool>


@tmat as we'll need a proper way of doing this at some point.

Can't we set MonoTool to a Bash file that invokes mono with the extra argument?

We get the MonoTool path as part of our build script

roslyn/eng/build.sh

Line 245 in b84562d

mono_path=`command -v mono`

So we'd need to dynamically emit that script to a temp path, then pass that in to MSBuild. That seems just as clunky as this?

I'm going to file an issue/PR against arcade to allow add an extensibility point here that we can tap into

Opened dotnet/arcade#1902

Why? The script can just call mono --debug $@. Mono will then be searched on $PATH

jaredpar · 2019-01-29T14:50:56Z

@chsienki

Looks like the docker kill command is wrong:

2019-01-29T00:25:57.5100274Z "docker kill" requires at least 1 argument.
2019-01-29T00:25:57.5101219Z See 'docker kill --help'.
2019-01-29T00:25:57.5101296Z

How can we tell what Mono version got installed here? I can't see the line with the date that you prevously sent in slack.

Note: I'm doing a rebuild of this just to see the effect on the Mono run / caching. Everything passed first time around.

chsienki · 2019-01-29T18:34:00Z

I'll take a look at the docker kill.

Due to rendering issues between VT and ADO web view the version info doesn't show up, but its there in the log. Download the txt version and search for Mono JIT compiler version

In reply to: 458567955 [](ancestors = 458567955)

agocke

I think we should file a bug to remove --no-cache when the machines have been cleared.

agocke · 2019-01-29T18:39:28Z

eng/docker/Mono/Dockerfile

+# Update previously installed mono-devel.
+# Cache bust ensures we'll always run the commands following regardless of docker cache status
+ARG CACHE_BUST=0
+RUN apt-get update && apt-get upgrade -y


Running this in the middle will disable caching for all subsequent statements. Is that what you intended?

chsienki · 2019-01-29T18:42:28Z

I think the kill command is ok actually. We grab the running PID and pass it to kill, but if it's already exited there's nothing to kill. We || true the kill to carry on regardless, so seems like that was the original intent. We could probably do it better, but seems fine for now.

In reply to: 458654516 [](ancestors = 458654516,458567955)

chsienki · 2019-01-29T19:27:13Z

I think we should file a bug to remove --no-cache when the machines have been cleared.

Filed #32916

jcouv · 2019-01-29T23:24:43Z

eng/targets/XUnit.targets

      </None>
    </ItemGroup>
  </Target>
+


jaredpar

update mono-devel in dockerfile, and don't cache later commands

55f3eaa

chsienki added Area-Compilers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Jan 28, 2019

chsienki added 2 commits January 28, 2019 15:54

Temporarily disable all cache in dockerfile

6c42a16

Handle quotes around --debug flag using xunit.targets

b3bda77

chsienki force-pushed the dockerfile_fix branch from 03fc902 to b3bda77 Compare January 28, 2019 23:56

chsienki added Area-Infrastructure and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Jan 29, 2019

jaredpar reviewed Jan 29, 2019

View reviewed changes

agocke reviewed Jan 29, 2019

View reviewed changes

chsienki mentioned this pull request Jan 29, 2019

Remove Mono CI --no-cache flag #32916

Closed

chsienki mentioned this pull request Jan 29, 2019

Allow passing of arguments to mono executable dotnet/arcade#1902

Merged

chsienki added 2 commits January 29, 2019 14:32

Use bash script to add --debug flag to mono instead

089bfd1

Fix mono invoke script path

c262369

jcouv reviewed Jan 29, 2019

View reviewed changes

eng/targets/XUnit.targets Outdated

</None>

</ItemGroup>

</Target>

Copy link

Member

jcouv Jan 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert?

chsienki added 2 commits January 29, 2019 15:29

Remove extra line

322f20c

Make mono script executable

0e836da

jaredpar approved these changes Jan 30, 2019

View reviewed changes

jaredpar merged commit 66d31a1 into dotnet:master Jan 30, 2019

chsienki mentioned this pull request Mar 20, 2019

Use TestRuntimeAdditionalArguments from Arcade: #34296

Merged

update mono-devel in dockerfile, and don't cache later commands #32885

update mono-devel in dockerfile, and don't cache later commands #32885

Uh oh!

Conversation

chsienki commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chsienki commented Jan 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Jan 29, 2019

Uh oh!

chsienki commented Jan 29, 2019

Uh oh!

agocke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chsienki commented Jan 29, 2019

Uh oh!

chsienki commented Jan 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chsienki commented Jan 28, 2019 •

edited

Loading