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

Extract bundled files when IncludeAllContentForSelfExtract is set #42435

Merged
merged 19 commits into from
Sep 30, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 1 addition & 1 deletion src/installer/corehost/cli/bundle/extractor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ void extractor_t::extract_new(reader_t& reader)
begin();
for (const file_entry_t& entry : m_manifest.files)
{
if (entry.needs_extraction())
if (m_manifest.is_netcoreapp3_compat_mode() || entry.needs_extraction())
Copy link
Member

Choose a reason for hiding this comment

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

It looks like there are a number of other places in this file that are wrong (locate, et al). I think a safer approach than trying to modify all the use sites is to have file_entry_t::read take an extra force_extract parameter, then pass it through to each file_entry_t constructor and store it in a field. Then we can check the field in needs_extraction and avoid changing all the call sites

Copy link
Member

Choose a reason for hiding this comment

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

Agreed - the problem is everywhere we call entry.needs_extraction, so it would be easier to bake it into the needs_extraction itself. It would feel cleaner if we stored something like extracted on each file entry.

{
extract(entry, reader);
}
Expand Down
8 changes: 5 additions & 3 deletions src/installer/corehost/cli/bundle/manifest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,18 @@

using namespace bundle;

manifest_t manifest_t::read(reader_t& reader, int32_t num_files)
manifest_t manifest_t::read(reader_t& reader, header_t& header)
elinor-fung marked this conversation as resolved.
Show resolved Hide resolved
{
manifest_t manifest;
manifest.m_netcoreapp3_compat_mode = header.is_netcoreapp3_compat_mode();

for (int32_t i = 0; i < num_files; i++)
for (int32_t i = 0; i < header.num_embedded_files(); i++)
{
file_entry_t entry = file_entry_t::read(reader);
manifest.files.push_back(std::move(entry));
manifest.m_need_extraction |= entry.needs_extraction();
manifest.m_files_need_extraction |= entry.needs_extraction();
}

manifest.m_files_need_extraction |= manifest.is_netcoreapp3_compat_mode();
return manifest;
}
21 changes: 17 additions & 4 deletions src/installer/corehost/cli/bundle/manifest.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <list>
#include "file_entry.h"
#include "header.h"

namespace bundle
{
Expand All @@ -16,16 +17,28 @@ namespace bundle
{
public:
manifest_t()
: m_need_extraction(false) {}
: m_files_need_extraction(false)
, m_netcoreapp3_compat_mode(false)
{
}

std::vector<file_entry_t> files;

static manifest_t read(reader_t &reader, int32_t num_files);
static manifest_t read(reader_t &reader, header_t &header);

bool files_need_extraction() { return m_need_extraction; }
bool files_need_extraction() const
{
return m_files_need_extraction;
}

bool is_netcoreapp3_compat_mode() const
{
return m_netcoreapp3_compat_mode;
}

private:
bool m_need_extraction;
bool m_files_need_extraction;
bool m_netcoreapp3_compat_mode;
elinor-fung marked this conversation as resolved.
Show resolved Hide resolved
};
}
#endif // __MANIFEST_H__
2 changes: 1 addition & 1 deletion src/installer/corehost/cli/bundle/runner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ StatusCode runner_t::extract()
m_runtimeconfig_json.set_location(&m_header.runtimeconfig_json_location());

// Read the bundle manifest
m_manifest = manifest_t::read(reader, m_header.num_embedded_files());
m_manifest = manifest_t::read(reader, m_header);

// Extract the files if necessary
if (m_manifest.files_need_extraction())
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
//

using System;
using System.IO;
using System.Linq;
using BundleTests.Helpers;
using Microsoft.DotNet.Cli.Build.Framework;
using Microsoft.DotNet.CoreSetup.Test;
using Microsoft.NET.HostModel.Bundle;
using Xunit;

namespace AppHost.Bundle.Tests
{
public class NetCoreApp3CompatModeTests : IClassFixture<NetCoreApp3CompatModeTests.SharedTestState>
{
private SharedTestState sharedTestState;

public NetCoreApp3CompatModeTests(SharedTestState fixture)
{
sharedTestState = fixture;
}

[Fact]
public void Bundle_Is_Extracted()
{
var fixture = sharedTestState.TestFixture.Copy();
string singleFile;
Bundler bundler = BundleHelper.BundleApp(fixture, out singleFile, BundleOptions.BundleAllContent);
var extractionBaseDir = BundleHelper.GetExtractionRootDir(fixture);

Command.Create(singleFile)
.CaptureStdErr()
.CaptureStdOut()
.EnvironmentVariable(BundleHelper.DotnetBundleExtractBaseEnvVariable, extractionBaseDir.FullName)
.Execute()
.Should()
.Pass()
.And
.HaveStdOutContaining("Hello World");
Copy link
Member

@agocke agocke Sep 18, 2020

Choose a reason for hiding this comment

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

Any way we could also confirm that the assemblies are actually loaded from the extraction location and not the bundle? Perhaps by printing the result of Assembly.Location or similar?

Copy link
Member

Choose a reason for hiding this comment

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

We should definitely test for this.

I think we always set BUNDLE_PROBE (regardless of 3.x compat mode), so bundle probing takes precedence during assembly loading - I could see some undesired behaviour happening as a result.

Copy link
Member

Choose a reason for hiding this comment

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

With the most recent changes bundle probe should return false for all queries - I checked that the runtime behavior in that case is the same as without bundle probe in the case of assembly resolution. Note that it's not the same in other cases - especially Environment.GetCommandLineArguments[0] should still return the .exe path, not the main .dll path.

I also checked all the other places in runtime where we somehow react to bundle probe, and they all look correct to me.


var extractionDir = BundleHelper.GetExtractionDir(fixture, bundler);
var bundleFiles = BundleHelper.GetBundleDir(fixture).GetFiles().Select(file => file.Name).ToArray();
var publishedFiles = Directory.GetFiles(BundleHelper.GetPublishPath(fixture), searchPattern: "*", searchOption: SearchOption.AllDirectories)
.Select(file => Path.GetFileName(file))
.Except(bundleFiles)
.ToArray();
var bundlerFiles = BundleHelper.GetBundleDir(fixture).GetFiles();
elinor-fung marked this conversation as resolved.
Show resolved Hide resolved
extractionDir.Should().HaveFiles(publishedFiles);
}

public class SharedTestState : IDisposable
{
public TestProjectFixture TestFixture { get; set; }
public RepoDirectoriesProvider RepoDirectories { get; set; }

public SharedTestState()
{
RepoDirectories = new RepoDirectoriesProvider();
TestFixture = new TestProjectFixture("StandaloneApp", RepoDirectories);
TestFixture
.EnsureRestoredForRid(TestFixture.CurrentRid, RepoDirectories.CorehostPackages)
.PublishProject(runtime: TestFixture.CurrentRid, outputDirectory: BundleHelper.GetPublishPath(TestFixture));
}

public void Dispose()
{
TestFixture.Dispose();
}
}
}
}