Skip to content

Issue with multiple indirect relative paths #71

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

Closed
maettu-this opened this issue Jun 20, 2017 · 14 comments
Closed

Issue with multiple indirect relative paths #71

maettu-this opened this issue Jun 20, 2017 · 14 comments

Comments

@maettu-this
Copy link
Collaborator

maettu-this commented Jun 20, 2017

Hi Oleg,

I though that all issues with relative paths have been solved, but I have just managed to trick the file resolver again when writing a script library.

Here's the situation:

  • Script.cs containing //css_import .\Library\Group\ItemContainerOfGroup.cs;
  • \Library
    • LibraryBasics.cs (not importing anything else)
    • \Group
      • ItemThatDerivesFromLibraryBasics.cs containing //css_import ..\LibraryBasics.cs;
      • ItemContainerOfGroup.cs containing //css_import .\ItemThatDerivesFromLibraryBasics.cs;

I agree that this is a somewhat complicated structure, but the library will contain more than 100 items, so I decided to group them in subfolders, each containing a container that collects all items of a group.

Now, when CSScript compiles Script.cs, it properly resolves ItemContainerOfGroup.cs as well as ItemThatDerivesFromLibraryBasics.cs, but then fails to resolve the LibraryBasics.cs base class, even though ItemThatDerivesFromLibraryBasics.cs properly refers to that file with a relative path.

The reason is that the file resolver (i.e. fileparser.ResolveFiles()) has none of the subfolders (neither "\Library" nor "\Library\Group") in its extraDirs array. This puzzles me, after all, CSScript has properly processed the //css_import .\Library\Group\ItemContainerOfGroup.cs; statement. Shouldn't such additional path somewhere be added down the chain to ResolveFiles()?

Adding //css_searchdir .. to ItemThatDerivesFromLibraryBasics.cs doesn't help.

But funnily, I can work around to issue by adding //css_searchdir .\Library\Group; to Script.cs, which will combine the //css_import ..\LibraryBasics.cs; to the correct location, resulting in the following combination of statements:

//css_searchdir .\Library\Group;
//css_import .\Library\Group\ItemContainerOfGroup.cs;

This is something we call "Doppel-Moppel" in German, i.e. something that is duplicated even though the duplication is not really necessary.

Best regards,
Matthias

@oleg-shilo
Copy link
Owner

Txs. Will look at it

@oleg-shilo
Copy link
Owner

It's an interesting one. It actually works as designed. How many times we've heard that? :)

> ...has none of the subfolders (neither "\Library" nor "\Library\Group") in its extraDirs array...

Correct. This is exactly what it should do.
CS-Script distinguishes two important qualities associated with importing: a script location and a probing directory. They work with each other but they do not substitute each other. Thus you can import (with relative or absolute path) the script that is located in 20 levels of sub-dirs and you should only expect the file to be imported but not 20 dirs added to the probing directories.

Thus if you did not specify explicitly any search dirs then all imports will be resolved with respect to the "root point", which is the current directory of the process. And this is exactly what is happening in your case. This also explains why //css_searchdir .\Library\Group helps.

Now, there is a solution to your problem. You can reset the "root point" of the import statement by using .\ preffix. Thus if you change in your code //css_import ..\LibraryBasics.cs; to //css_import .\..\LibraryBasics.cs; all your files will be imported correctly. The solution is attached.

image

issue #71.zip

@maettu-this
Copy link
Collaborator Author

Yes, the suggested change to //css_import .\..\LibraryBasics.cs; has indeed fixed the issue! I can live with this, even though I think it is not a common and user-friendly syntax. DOS as well as UNIX fully understand ".." without an additional "." prefix. So I doubt that users find this solution better than I did ;-) Also note that I haven't found that hint anywhere in the help.

@oleg-shilo
Copy link
Owner

I think there is a slight misunderstanding here.

The single-dot prefix is not a solution for your problem as I am not convinced there is a problem to solve.

I only recommended to use it because it can help you to avoid specifying probing directories (search dirs) explicitly. Something that you wanted. However a proper reliable approach is in fact to specify search dirs explicitly.

You proposal to allow search dirs being auto-added while resolving the files, despite being attractive, can lead to the unpredictable behavior. Simply because in this case the overall outcome of the assembly probing is non-deterministic. Thus unfortunately the proposal cannot be accepted.

> DOS as well as UNIX fully understand ".." without an additional "." prefix.
Yes and CS-Script fully relies on the underlying FS path notation. Though both Win and Linux use a single-dot extra prefix to specify "current directory". Thus the use of this prefix is quite common.

> Also note that I haven't found that hint anywhere in the help.
The CS-Script command line help has it described (the last sentence below):

Q:\Extras\Utils\Console2>cscs -syntax
**************************************
Script specific syntax
**************************************
Engine directives:
------------------------------------
//css_include <file>;

Alias - //css_inc
file - name of a script file to be included at compile-time.

This directive is used to include one script into another one.It is a logical equivalent 
of '#include' in C++. This directive is a simplified version of //css_import.
If a relative file path is specified with single-dot prefix it will be automatically converted 
into the absolute path with respect to the location of the file containing the directive 
being resolved.
...

However, you are right the online help does not contain the "single-dot prefix" description. It will be fixed asap.

Thank you,
Oleg

@maettu-this
Copy link
Collaborator Author

I fully understand that "allow search dirs being auto-added" can lead to undesired side effects, so I understand that this is a no-go.

But:

Why not allow the single-dot behavior for double-dot prefixes as well? I.e. "If a relative file path is specified with single-dot or double-dot prefix it will be automatically converted into the absolute path with respect to the location of the file containing the directive being resolved."? I think that would be very intuitive, since DOS/UNIX/... also accept "." as well as ".." when starting a relative path.

@oleg-shilo
Copy link
Owner

I actually like your idea even though it mixes up two different concepts

  • "current dir" as current directory of the host process.
  • "current dir" as a location (root) directory of the script with the //css_include directive.

Allowing single-dot behavior for double-dot prefixes unconditionally has a side effect as it would complicate referencing scripts from global highly structured folders. It will also potentially upset the users who already relying on "single vs double" being handled differently.

But... I am not sure how many users indeed capitalize on the current flexibility of "single vs double". And I do agree that having the unified probing algorithm is more intuitive. Thus I have implemented your proposal in the latest codebase though as an option. The pre-release v3.26.1.0 does not have it but when it is to be finally released it will contain the change.

In order to activate it you will need to have it activated from the config file:

...
  <enableDbgPrint>True</enableDbgPrint>
  <resolveRelativeFromParentScriptLocation>True</resolveRelativeFromParentScriptLocation>
</CSSConfig>

Initially it will be set to false by default but the following release will have it defaulted to true.

@maettu-this
Copy link
Collaborator Author

maettu-this commented Jul 20, 2017

Hi Oleg,

I have upgraded to 3.27.0 and tried to verify issue #71. Unfortunately it doesn't work as expected, I am still getting a FileNotFoundException:

<Workspace>\CSScript\Issue #71>cscs.exe .\css_importTest\Me\TestScript.cs
C# Script execution engine. Version 3.27.0.0.
Copyright (C) 2004-2017 Oleg Shilo.

Error: Specified file could not be compiled.

System.IO.FileNotFoundException: Could not find file "..\RedirectParentImpLib.cs".
Ensure it is in one of the CS-Script search/probing directories.
   at CSScriptLibrary.FileParser.ResolveFilesDefault(String file, String[] extraDirs, Boolean throwOnError)
   at CSScriptLibrary.FileParser.ResolveFile(String file, String[] extraDirs, Boolean throwOnError)
   at CSScriptLibrary.FileParser..ctor(String fileName, ParsingParams prams, Boolean process, Boolean imported, String[] searchDirs, Boolean throwOnError)
   at CSScriptLibrary.ScriptParser.ProcessFile(ScriptInfo fileInfo)
   at CSScriptLibrary.ScriptParser.ProcessFile(ScriptInfo fileInfo)
   at CSScriptLibrary.ScriptParser.Init(String fileName, String[] searchDirs)
   at csscript.CSExecutor.Compile(String scriptFileName)
   at csscript.CSExecutor.ExecuteImpl()

<Workspace>\CSScript\Issue #71>

When prefixing the css_import paths with a .\ everything works perfectly:

<Workspace>\CSScript\Issue #71>cscs.exe .\css_importTest\Me\TestScript.cs
C# Script execution engine. Version 3.27.0.0.
Copyright (C) 2004-2017 Oleg Shilo.

This is "CSScript.Issue71.css_importTest.Me.LocalImpLib".
This is "CSScript.Issue71.css_importTest.Me.Child.ChildImpLib".
This is "CSScript.Issue71.css_importTest.Me.Redirect.Child.RedirectChildImpLib". Invoked via "CSScript.Issue71.css_importTest.Me.Redirect.RedirectImpLib" and 
This is "CSScript.Issue71.css_importTest.Me.RedirectParentImpLib". Invoked via "CSScript.Issue71.css_importTest.Me.Redirect.RedirectImpLib".
This is "CSScript.Issue71.css_importTest.ParentImpLib".
This is "CSScript.Issue71.css_importTest.Redirect.Child.ParentRedirectChildImpLib". Invoked via "CSScript.Issue71.css_importTest.Redirect.ParentRedirectImpLib" and This is "CSScript.Issue71.css_importTest.ParentRedirectParentImpLib". Invoked via "CSScript.Issue71.css_importTest.Redirect.ParentRedirectImpLib".
This is "CSScript.Issue71.css_importTest.Peer.PeerImpLib".
This is "CSScript.Issue71.css_importTest.Peer.Child.PeerChildImpLib".
This is "CSScript.Issue71.css_importTest.Peer.Redirect.Child.PeerRedirectChildImpLib". Invoked via "CSScript.Issue71.css_importTest.Peer.Redirect.PeerRedirectImpLib" and This is "CSScript.Issue71.css_importTest.Peer.PeerRedirectParentImpLib". Invoked via "CSScript.Issue71.css_importTest.Peer.Redirect.PeerRedirectImpLib".

<Workspace>\CSScript\Issue #71>

I am attaching my test scripts. You can reproduce the issue as follows:

  1. Extract the .zip into a separate folder
  2. Call cscs.exe .\css_importTest\Me\TestScript.cs => OK
  3. Remove all // ".\" prefix is required to work around CSScript issue #71. (3 occurrences)
  4. Call cscs.exe .\css_importTest\Me\TestScript.cs again => NOK

I have a second bunch of test scripts verifying css_reference. It basically works the very same, just using .dll files instead of .cs files. I am attaching this as well for reference, though I ran into #79 when trying to build some of the .dll's.

In my own test environment I actually have both tests in the same folder structure, but for this issue description I have split them to simplify things a bit.

Best regards,
Matthias

css_importTest.zip
css_referenceTest.zip

oleg-shilo added a commit that referenced this issue Jul 27, 2017
 - Added `ResolveRelativeFromParentScriptLocation` for ALL non absolute paths in `//css_import`
@oleg-shilo
Copy link
Owner

Hi Matthias,

The current release has this feature enabled only for imports staring with "../".

if (ResolveRelativeFromParentScriptLocation && statement.Length > 1 && statement.StartsWith(".."))
    statement = Path.Combine(Path.GetDirectoryName(parentScript), statement);

I just wasn't sure about applying this approach to the "naked paths" (relative path that starts with a character) like in your samples.

I have to say, it was rather an intuitive but not a logical decision. And after looking at your samples I am convinced that the approach needs to be extended the whole way. Thus 0d398ee commit delivers the support for "naked files" as well. You can test it without waiting for the release. The binaries can be found here: https://github.com/oleg-shilo/cs-script/tree/master/Source/Build.

Just ensure you have enabled ResolveRelativeFromParentScriptLocation:

cscs -config:set:ResolveRelativeFromParentScriptLocation=true

There is one extra point I want to make. You have attached css_referenceTest.zip file, which suggests you expect this new variation of the probing mechanism to work for referencing (//css_ref) as well. If it is the case then please be aware that referencing will need to stay as is.

The reason for this is that this new handling "naked paths" in the algorithm makes the feature incompatible with referencing.

No "naked assemblies" should be converted into absolute path as it would really screw the C# compiler. Remember, .NET compilers reserve "naked paths" for GAC assemblies. This is where that single dot makes perfect sense as it makes possible to distinguish between local (.\system.dll) and GAC assembly (system.dll).

This in tern makes me question my earlier objective of "having the unified probing algorithm". It is clear to me now that the "auto-doting" naked paths can only work reliable for importing but not referencing. Thus if even referencing ever gets "auto-doting" support, I think it is safer to keep ResolveRelativeFromParentScriptLocation disabled in the settings by default.

Thank you,
Oleg

@maettu-this
Copy link
Collaborator Author

Upps, I thought I had earlier commented on your question, but somehow it got lost... So again:

css_referenceTest.zip I have attached just for reference, in case you can make use of it. Note that css_referenceTest.zip fully passes with release 3.27.0, so nothing needs to be changed with //css_ref in my opinion.

And, I personally prefer keeping ResolveRelativeFromParentScriptLocation enabled, since I still consider ..\ very intuitive.

Best regards,
Matthias

oleg-shilo added a commit that referenced this issue Aug 4, 2017
Minor usability improvements

* Added System.dll auto-referencing on EnableDbgPrint:true to allow regular expressions to be used in auto-injected dbg.cs.
* Issue #78: Beautify command line help output
* Issue #71: Issue with multiple indirect relative paths
* Added ResolveRelativeFromParentScriptLocation for ALL non absolute paths in //css_import
@maettu-this
Copy link
Collaborator Author

Hmm, is there still something wrong are am I verifying wrongly? I still get the same error with the test scripts:

<Workspace>\CSScript\Issue #71>cscs.exe .\css_importTest\Me\TestScript.cs
C# Script execution engine. Version 3.27.1.0.
Copyright (C) 2004-2017 Oleg Shilo.

Error: Specified file could not be compiled.

System.IO.FileNotFoundException: Could not find file "..\RedirectParentImpLib.cs".
Ensure it is in one of the CS-Script search/probing directories.
   at CSScriptLibrary.FileParser.ResolveFilesDefault(String file, String[] extraDirs, Boolean throwO
nError)
   at CSScriptLibrary.FileParser.ResolveFile(String file, String[] extraDirs, Boolean throwOnError)
   at CSScriptLibrary.FileParser..ctor(String fileName, ParsingParams prams, Boolean process, Boolea
n imported, String[] searchDirs, Boolean throwOnError)
   at CSScriptLibrary.ScriptParser.ProcessFile(ScriptInfo fileInfo)
   at CSScriptLibrary.ScriptParser.ProcessFile(ScriptInfo fileInfo)
   at CSScriptLibrary.ScriptParser.Init(String fileName, String[] searchDirs)
   at csscript.CSExecutor.Compile(String scriptFileName)
   at csscript.CSExecutor.ExecuteImpl()

<Workspace>\CSScript\Issue #71>

When keeping the ".\" prefix at the three following locations, all works fine again:

<Workspace>\CSScript\Issue #71\css_importTest\Me\Redirect\RedirectImpLib.cs(3)
<Workspace>\CSScript\Issue #71\css_importTest\Peer\Redirect\PeerRedirectImpLib.cs(3)
<Workspace>\CSScript\Issue #71\css_importTest\Redirect\ParentRedirectImpLib.cs(3)

Something at first puzzled me:

  1. By default cscs.exe -config gives me ResolveRelativeFromParentScriptLocation: False.
  2. Setting it to true creates the config file, and all works fine even without the ".\" prefixes at the three locations.
  3. Deleting the config file gives me ResolveRelativeFromParentScriptLocation: False again, and all works still fine even without the ".\" prefixes at the three locations.
  4. Deleting the CSScript cache, and it doesn't work anymore.

But of course this is all expected behavior of the cache. Still, as I mentioned earlier, I'd prefer a default setting of true, since I prefer to keep default settings whenever possible and I think the same behavior on ".\" and "..\" would be consistent.

So, was it intended to keep the default setting to false, or was this something that went wrong on releasing 3.27.1.0?

@maettu-this
Copy link
Collaborator Author

For the case it was indeed intended to keep false, I have modified my hosting application such it uses CSScript.CompileWithConfig() instead of CSScript.CompileFile(), specifying settings with ResolveRelativeFromParentScriptLocation = true;. This works.

@oleg-shilo
Copy link
Owner

I am sorry, my wording in my previous post wasn't definitive enough:

Thus if even referencing ever gets "auto-doting" support, I think it is safer to keep ResolveRelativeFromParentScriptLocation disabled in the settings by default.

What I meant to say was that ResolveRelativeFromParentScriptLocation is going to stay "disabled" by default.

I know it goes against you personal preference but in this case I decided to make the "consistency" a primary goal even by slightly sacrificing the usability in the edge cases like yours.

The decision is really based on a simple risk/benefit assessment. Particularly because the mechanism for a simple overwriting the default behavior is available.


I am glad you have figured out the way around. But you can do it even in a simpler way. Just call it once and the setting will be set globally:

CSScript.GlobalSettings.ResolveRelativeFromParentScriptLocation = true;

@maettu-this
Copy link
Collaborator Author

I understand that the risks are too high for changing something that many users are likely to rely upon.

Thanks for the hint with GlobalSettings. However, since my test environment compiles and loads files from static methods (NUnit), that setting would have to be configured by a static class that for sure is initialized before the test code. The order of static initializers is not given and I would have to implement some additional initialization logic. Thus, I will stick to use CSScript.CompileWithConfig() and CSScript.LoadWithConfig() in order to have a fully predictable and simple initialization sequence.

By the way, I have just noticed a minor inconsistency with CSScript's methods:
CSScript.Compile() is deprecated and has been replaced by CSScript.CompileFile()
-vs-
CSScript.Load() is still active and there is no CSScript.LoadFile().

So, this item #71 is verified and completed for me. Thanks!

@oleg-shilo
Copy link
Owner

... CSScript.Load() is still active and there is no CSScript.LoadFile().

Fair point. Done. Will be delivered in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants