Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Xamarin.Android.Build.Tasks] timestamp fixes for incremental builds #1930

Merged
merged 1 commit into from
Jul 13, 2018
Merged

[Xamarin.Android.Build.Tasks] timestamp fixes for incremental builds #1930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,8 @@ void UpdateWhenChanged (string path, Action<Stream> generator)
var np = path + ".new";
using (var o = File.OpenWrite (np))
generator (o);
Files.CopyIfChanged (np, path);
MonoAndroidHelper.CopyIfChanged (np, path);
MonoAndroidHelper.SetLastAccessAndWriteTimeUtc (path, DateTime.UtcNow, Log);
Copy link
Member Author

Choose a reason for hiding this comment

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

@dellis1972 I was looking at this code. Do we really need this .new file that we delete afterward?

Can we just write to path, update the timestamp, and that's it?

Copy link
Contributor

Choose a reason for hiding this comment

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

We do this allot. If we generate code we don't write directly to the file, we instead use a temp file and use 'CopyIfChanged' this ensures that we don't update files that we don't need to. That said in this case we are setting the date time.. so that kinda makes it redundant.

Might be worth testing to see what impact it has.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I think using the .new file might be right.

Consider:

  • generator(o) writes to the original file and fails with some error
  • We could have a 1/2 complete file with a newer timestamp, and the target wouldn't run again

File.Delete (np);
}
}
Expand Down
7 changes: 4 additions & 3 deletions src/Xamarin.Android.Build.Tasks/Tasks/LinkAssemblies.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,16 +152,18 @@ bool Execute (DirectoryAssemblyResolver res)
foreach (var assembly in ResolvedAssemblies) {
var copysrc = assembly.ItemSpec;
var filename = Path.GetFileName (assembly.ItemSpec);
var assemblyDestination = Path.Combine (copydst, filename);

if (options.LinkNone) {
if (skiplist.Any (s => Path.GetFileNameWithoutExtension (filename) == s)) {
// For skipped assemblies, skip if there is existing file in the destination.
// We cannot just copy the linker output from *current* run output, because
// it always renew the assemblies, in *different* binary values, whereas
// the dll in the OptionalDestinationDirectory must retain old and unchanged.
if (File.Exists (Path.Combine (copydst, filename)))
if (File.Exists (assemblyDestination)) {
MonoAndroidHelper.SetLastAccessAndWriteTimeUtc (assemblyDestination, DateTime.UtcNow, Log);
continue;
copysrc = assembly.ItemSpec;
}
} else {
// Prefer fixup assemblies if exists, otherwise just copy the original.
copysrc = Path.Combine (OutputDirectory, filename);
Expand All @@ -171,7 +173,6 @@ bool Execute (DirectoryAssemblyResolver res)
else if (!MonoAndroidHelper.IsForceRetainedAssembly (filename))
continue;

var assemblyDestination = Path.Combine (copydst, filename);
if (MonoAndroidHelper.CopyIfChanged (copysrc, assemblyDestination)) {
MonoAndroidHelper.SetLastAccessAndWriteTimeUtc (assemblyDestination, DateTime.UtcNow, Log);
}
Expand Down
17 changes: 16 additions & 1 deletion src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ public void CheckTimestamps ()
var output = Path.Combine (Root, b.ProjectDirectory, proj.OutputPath);
if (Directory.Exists (output))
Directory.Delete (output, true);
Assert.IsTrue (b.Build (proj), "Build should have succeeded.");
Assert.IsTrue (b.Build (proj), "first build should have succeeded.");

//Absolutely non of these files should be *older* than the starting time of this test!
var files = Directory.EnumerateFiles (intermediate, "*", SearchOption.AllDirectories).ToList ();
Expand All @@ -142,6 +142,21 @@ public void CheckTimestamps ()
var info = new FileInfo (file);
Assert.IsTrue (info.LastWriteTimeUtc > start, $"`{file}` is older than `{start}`, with a timestamp of `{info.LastWriteTimeUtc}`!");
}

//Build again after a code change, checking a few files
proj.MainActivity = proj.DefaultMainActivity.Replace ("clicks", "CLICKS");
proj.Touch ("MainActivity.cs");
start = DateTime.UtcNow;
Assert.IsTrue (b.Build (proj), "second build should have succeeded.");

foreach (var file in new [] { "typemap.mj", "typemap.jm" }) {
var info = new FileInfo (Path.Combine (intermediate, "android", file));
Assert.IsTrue (info.LastWriteTimeUtc > start, $"`{file}` is older than `{start}`, with a timestamp of `{info.LastWriteTimeUtc}`!");
}

//One last build with no changes
Assert.IsTrue (b.Build (proj), "third build should have succeeded.");
Assert.IsTrue (b.Output.IsTargetSkipped ("_LinkAssembliesNoShrink"), "`_LinkAssembliesNoShrink` should be skipped!");
}
}

Expand Down