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

is there any reasons that merging working dir not app base dir into probing path? #3708

Closed
liesauer opened this issue Aug 1, 2019 · 10 comments
Labels
area-Host question Answer questions and provide assistance, not an issue with source code or documentation.
Milestone

Comments

@liesauer
Copy link
Author

liesauer commented Aug 1, 2019

it should be supposed to merge app base dir.

@vitek-karas
Copy link
Member

I don't know - but it's unlikely to change now. Changing it would be a significant breaking change. Also note that it's not just in the fx_muxer you linked above, the same behavior applies to other places - basically everywhere the call to pal::realpath is made.

@liesauer
Copy link
Author

liesauer commented Aug 2, 2019

i edited this line https://github.com/dotnet/core-setup/blob/6db018b7fc5c104810f5a53b538841004e099713/src/corehost/cli/fxr/fx_muxer.cpp#L217

to

pal::string_t probe_path = host_info.dotnet_root.c_str() + path;

everything seems working properly, no pal::realpath changes needed, so i don't think this solution is a breaking change.

@vitek-karas
Copy link
Member

There are multiple ways to specify additional probing paths, one such way is command line: --additionalprobingpath. So if somebody has a script which does something along these lines:

cd MyExtensionsDirectory
/absolute/path/to/app/myapp --additionalprobingpath LoggingExtension

With your proposed change such script would stop working. Before your change it would look for the LoggingExtension under the MyExtensionsDirectory. After your change it would look for it in the myapp.

Similar argument can be made about other ways to specify additional probing paths. I personally agree that at least those probing paths specified in the .runtimeconfig.dev.json should be considered relative to the app. On the other hand the command line option would make sense to be relative to current directory (since that's how most command line arguments work in general if they specify relative paths). Regardless, the current behavior is at least consistent across multiple ways to specify the same thing... and the breaking change nature of such modification would make this a very hard sell.

If your mains scenario is ways to reorganize files in the app layout via modifications to .deps.json, then I think the best way is to fix #3525. I believe it's possible to do so without introducing breaking changes, or with only a very small risk of breaking anything. One such way would be to keep the code as is, and when we probe for the potential location of files, append to the end of such probes yet another one which would correctly obey the relative path specified in the .deps.json and treat it as relative to each probing location (app base is always one of them). This would mean that if the file can be found with existing behavior it would be found there first. Only if it can't be found (and thus would lead to an error), in the new behavior we would also look into the "new location".

@liesauer
Copy link
Author

liesauer commented Aug 2, 2019

right, probing paths specified in the .runtimeconfig.json should be considered relative to the app.

@liesauer
Copy link
Author

liesauer commented Aug 2, 2019

Update.
https://github.com/dotnet/core-setup/blob/6db018b7fc5c104810f5a53b538841004e099713/src/corehost/cli/runtime_config.cpp#L98-L109

        if (probe_paths->second.is_string())
        {
            m_probe_paths.insert(m_probe_paths.begin(), get_directory(m_path).c_str() + probe_paths->second.as_string());
        }
        else
        {
            const auto& arr = probe_paths->second.as_array();
            for (auto iter = arr.rbegin(); iter != arr.rend(); iter++)
            {
                m_probe_paths.push_front(get_directory(m_path).c_str() + iter->as_string());
            }
        }

@vitek-karas
Copy link
Member

Thanks - that makes sense. But we're still facing the fact that it would be a breaking change.

I'm curious, do you think #3525 would fix your scenario?

@liesauer
Copy link
Author

liesauer commented Aug 2, 2019

partially, it only works perfectly when combo with additionalProbingPaths in runtimeconfig.json.

@vitek-karas
Copy link
Member

Can I ask why is that? If we fix #3525 you should be able to specify any path relative to app base in the .deps.json. In which case why would you need additionalProbingPaths?

@liesauer
Copy link
Author

liesauer commented Aug 3, 2019

https://github.com/dotnet/core-setup/blob/6db018b7fc5c104810f5a53b538841004e099713/src/corehost/cli/hostpolicy/deps_resolver.cpp#L278-L366

this method also base on the probe config, i don't think we can avoid modifying probing path part.

@msftgits msftgits transferred this issue from dotnet/core-setup Jan 30, 2020
@msftgits msftgits added this to the Future milestone Jan 30, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Host question Answer questions and provide assistance, not an issue with source code or documentation.
Projects
None yet
Development

No branches or pull requests

3 participants