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

Jump to bottom

Update runtimeconfig.json and deps.json paths when these break past the MAX_PATH threshold #56224

Merged
merged 6 commits into from
Jul 27, 2021
Merged

Update runtimeconfig.json and deps.json paths when these break past the MAX_PATH threshold #56224

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2edc183
Update runtimeconfig paths if we exceed MAX_PATH
Jul 22, 2021
bc795aa
Add test
Jul 23, 2021
e4468c9
PR feedback
Jul 26, 2021
072492d
Get apphost for all platforms
Jul 26, 2021
3c01e0d
Probe bundle first
Jul 26, 2021
3c2f128
Check for empty path upfront
Jul 27, 2021
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
30 changes: 30 additions & 0 deletions src/installer/tests/HostActivation.Tests/PortableAppActivation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,36 @@ public void AppHost_FrameworkDependent_GlobalLocation_Succeeds(bool useRegistere
}
}

[Fact]
public void RuntimeConfig_FilePath_Breaks_MAX_PATH_Threshold()
{
var project = sharedTestState.PortableAppFixture_Published
.Copy();

var appExeName = Path.GetFileName(project.TestProject.AppExe);
var outputDir = project.TestProject.OutputDirectory;

// Move the portable app to a path such that the length of the executable's fullpath
// is just 1 char behind MAX_PATH (260) so that the runtimeconfig(.dev).json files
// break this threshold. This will cause hostfxr to normalize these paths -- here we
// are checking that the updated paths are used.
var dirName = new string('a', 259 - outputDir.Length - appExeName.Length - 2);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: In theory this would break if the location of the repo was in a too deep. Maybe we should create a folder in temp (which is basically guaranteed to the short enough on all systems).

We do have other tests creating long paths, but those don't need specific path length, they just need path longer than something. This test needs a specific path length and so the arithmetic above could end up with negative size (and the test will fail).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this totally relies on the user having the repo on a path not so far from root. Wasn't sure if it was good to put this on %TMP%, now I remember that we do that for bundling tests... changing this.

var newDir = Path.Combine(outputDir, dirName);
var appExe = Path.Combine(newDir, appExeName);
Debug.Assert(appExe.Length == 259);
Directory.CreateDirectory(newDir);
foreach (var file in Directory.GetFiles(outputDir, "*.*", SearchOption.TopDirectoryOnly))
File.Copy(file, Path.Combine(newDir, Path.GetFileName(file)));

Command.Create(appExe)
.DotNetRoot(project.BuiltDotnet.BinPath)
.EnableTracingAndCaptureOutputs()
.MultilevelLookup(false)
.Execute()
.Should().Pass()
.And.HaveStdOutContaining("Hello World");
}

[Fact]
public void ComputedTPADoesntEndWithPathSeparator()
{
Expand Down
4 changes: 2 additions & 2 deletions src/native/corehost/deps_format.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ bool deps_json_t::has_package(const pal::string_t& name, const pal::string_t& ve
bool deps_json_t::load(bool is_framework_dependent, const pal::string_t& deps_path, const rid_fallback_graph_t& rid_fallback_graph)
{
m_deps_file = deps_path;
m_file_exists = bundle::info_t::config_t::probe(deps_path) || pal::file_exists(deps_path);
m_file_exists = pal::realpath(&m_deps_file, true) || bundle::info_t::config_t::probe(m_deps_file);
elinor-fung marked this conversation as resolved.
Show resolved Hide resolved

json_parser_t json;
if (!m_file_exists)
Expand All @@ -449,7 +449,7 @@ bool deps_json_t::load(bool is_framework_dependent, const pal::string_t& deps_pa
return true;
}

if (!json.parse_file(deps_path))
if (!json.parse_file(m_deps_file))
{
return false;
}
Expand Down
6 changes: 3 additions & 3 deletions src/native/corehost/runtime_config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,9 +334,9 @@ bool runtime_config_t::ensure_dev_config_parsed()
trace::verbose(_X("Attempting to read dev runtime config: %s"), m_dev_path.c_str());

pal::string_t retval;
if (!pal::file_exists(m_dev_path))
if (!pal::realpath(&m_dev_path, true))
{
// Not existing is valid.
// It is valid for the runtimeconfig.dev.json to not exist.
return true;
}

Expand Down Expand Up @@ -402,7 +402,7 @@ bool runtime_config_t::ensure_parsed()
trace::verbose(_X("Did not successfully parse the runtimeconfig.dev.json"));
}

if (!bundle::info_t::config_t::probe(m_path) && !pal::file_exists(m_path))
if (!pal::realpath(&m_path, true) && !bundle::info_t::config_t::probe(m_path))
{
// Not existing is not an error.
return true;
Expand Down