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.

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

Implement PreserveCompilationContext #112

Merged
merged 6 commits into from
Sep 2, 2016

Conversation

eerhardt
Copy link
Member

Implementing PreserveCompilationContext during publish.

See https://github.com/dotnet/cli/issues/3932 for how this feature works, and why.

There is one thing left to do: #111

@brthor @livarcocc @natidea @nguerrera

/cc @dotnet/project-system

@davidfowl
Copy link
Member

/cc @anurse @pakrym @pranavkm

@eerhardt eerhardt force-pushed the PreserveCompilationContext branch from 3e5886e to 1e256f3 Compare September 1, 2016 01:35
@eerhardt eerhardt force-pushed the PreserveCompilationContext branch from 1e256f3 to e905acd Compare September 1, 2016 01:39
{
// write the result file out on failure for easy comparison

using (JsonTextWriter writer = new JsonTextWriter(File.CreateText($"result-{baselineFileName}.deps.json")))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be dumped on the screen instead? Thinking about CI here? Or maybe, have it dumped on a file only for dev machines but always show it in the console?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is always shown in the console. The exception thrown by .Should().BeEquivalentTo() contains both the baseline and the result.

However that formatting is near un-readable because of https://github.com/dennisdoomen/FluentAssertions/issues/453. And also "diffing" between the two is impossible when the object is larger than a few properties.


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

Copy link
Contributor

Choose a reason for hiding this comment

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

All right, but do we care about this file being generated in CI like that? I wonder if this is a bad pattern to pick up.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we could capture the bin directory in CI, then yes. This would greatly help debugging failures of these tests.

@analogrelay
Copy link
Contributor

LGTM, would like @pakrym to take a look at the deps files to make sure the syntax is right, but otherwise looks OK.

@natidea
Copy link
Contributor

natidea commented Sep 1, 2016

                if (Path.GetFileName(filePath) == "_._")

This statement can also be replaced with IsPlaceholderFile


Refers to: src/Tasks/Microsoft.NETCore.Build.Tasks/ResolvePackageDependencies.cs:333 in e905acd. [](commit_id = e905acd, deletion_comment = False)

@natidea
Copy link
Contributor

natidea commented Sep 1, 2016

                if (Path.GetFileName(file) == "_._")

This statement can also be replaced with IsPlaceholderFile


Refers to: src/Tasks/Microsoft.NETCore.Build.Tasks/ResolvePackageDependencies.cs:210 in e905acd. [](commit_id = e905acd, deletion_comment = False)

@natidea
Copy link
Contributor

natidea commented Sep 1, 2016

LGTM

{
// TODO: What other information about the current project needs to be included here? - https://github.com/dotnet/sdk/issues/12

RuntimeAssetGroup[] runtimeAssemblyGroups = new[] { new RuntimeAssetGroup(string.Empty, $"{projectName}.dll") };
Copy link
Contributor

Choose a reason for hiding this comment

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

you are teasing me with this right.... Urgh... can't.... resits...
NIT: var.

@livarcocc
Copy link
Contributor

:shipit:

@eerhardt eerhardt force-pushed the PreserveCompilationContext branch 2 times, most recently from b2b0c52 to 46f3f14 Compare September 2, 2016 03:17
@eerhardt
Copy link
Member Author

eerhardt commented Sep 2, 2016

@dotnet-bot test this please

@eerhardt eerhardt closed this Sep 2, 2016
@eerhardt eerhardt reopened this Sep 2, 2016
@eerhardt eerhardt force-pushed the PreserveCompilationContext branch from 46f3f14 to a22eb92 Compare September 2, 2016 15:32
@eerhardt eerhardt merged commit 2509f18 into dotnet:master Sep 2, 2016
@eerhardt eerhardt deleted the PreserveCompilationContext branch September 2, 2016 15:45
nguerrera pushed a commit that referenced this pull request Oct 10, 2016
sbomer pushed a commit to sbomer/sdk that referenced this pull request Sep 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants