Enhance and refactor URIs resolving logic #11

diegoferigo · 2024-02-23T16:10:26Z

Main changes:

Resolving now fail if the file:// path (that is assumed to be absolute) does not exist.
If the URI has no scheme, file:// is used by default.

After these changes, the resolved URI is ensured to be an existing file. Before, there were cases in which the resolved path was a non-existent file. Now downstream projects that consume the resulting pathlib.Path object can prevent to check the existence of the resolved file.

Furthermore:

Added new tests.
Exposed the supported schemes so that downstream projects can check them.
Exposed the supported environment variables so that downstream projects can check them.

Superseeds #10.

traversaro · 2024-02-24T14:21:15Z

Thanks a lot! I am not a big fan of erroring on non-existing file:// uri, ma indeed this is coherent with how package:// and model:// work.

traversaro · 2024-02-24T14:21:33Z

Mhh, CI was probably disabled, let me enable it.

traversaro · 2024-02-24T14:24:02Z

There is a CI warning on Linux and macOS

=============================== warnings summary ===============================
../../../micromamba/envs/testenv/lib/python3.11/site-packages/_pytest/config/__init__.py:1457
  /home/runner/micromamba/envs/testenv/lib/python3.11/site-packages/_pytest/config/__init__.py:1457: PytestConfigWarning: No files were found in testpaths; consider removing or adjusting your testpaths configuration. Searching recursively from the current directory instead.
    self.args, self.args_source = self._decide_args(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
========================= 3 passed, 1 warning in 0.02s =========================

is this expected?

Instead, on Windows the test fail.

traversaro · 2024-02-24T14:27:15Z

src/resolve_robotics_uri_py/resolve_robotics_uri_py.py

-        if (len(model_filenames) > 1):
-            warnings.warn(f"resolve-robotics-uri-py: Multiple files ({pathlist_list_to_string(model_filenames)}) found for uri \"{uri}\", returning the first one.")
+        # Ensure the URI path is absolute
+        uri_path = uri_path if uri_path.startswith("/") else f"/{uri_path}"


This logic is wrong on Windows, and probably the reason why the tests are failing on Windows. Can't we just pass uri_path to pathlib.Path ? If it is not an absolute path, an error will be raised, as it should be.

Ah I see, the problem is that you removed the special handling in model_filenames.append(uri.replace("file:/", "")) for the file:/. Any specific reason to do that?

Let's consider an URI in the form file://absolute/path/to/file.txt, and the URI without scheme /absolute/path/to/file.txt. With this PR, URIs without schemes fall back to file://.

When the scheme is stripped, the path becomes either absolute/path/to/file.tx or remains /absolute/path/to/file.txt. At least in Ubuntu, I need to add a trailing /.

Before this PR, the logic was always removing file:/ (note the missing trailing /). Leaving an extra / in case the path was already absolute. I'll make some test in CI on windows to figure out what's wrong there.

When the scheme is stripped, the path becomes either absolute/path/to/file.tx or remains /absolute/path/to/file.txt. At least in Ubuntu, I need to add a trailing /.

I am a bit confused. If the scheme is file:/, so if you remove it you remain with /absolute/path/to/file.txt, in which context you will have absolute/path/to/file.txt after stripping file:/ ?

I never strip file:/. The new logic always strips file:// and adds a leading / to the resulting path if it doesn't start already with /.

Actually, I think that the logic (and the test) were at least incomplete also before. It seems that before it was kind of assuming that file uri in Windows were file:/C:/etc/etc, but according to https://datatracker.ietf.org/doc/rfc8089/ also file:C:/etc/etc and file:///C:/etc/etc are supported, and actually the latter is the one used in https://en.wikipedia.org/wiki/File_URI_scheme . However, for sure it seems that here instead now file://C:/etc/etc is used/assumed, and that from what I understand is not a failure file: uri.

Good to know, I think we're not yet close to support the full file:// specification. Maybe we can try to be 100% compatible in a new PR.

To be honest, I am not even sure we want to be, whatever subset works fine for the average robotics users for us is ok.

However, if I am not wrong in https://github.com/ami-iit/resolve-robotics-uri-py/pull/11/files#diff-1e7414883e02e7f25fddd798742a4cd9cc87486af18923f0efb6940c30f49ebeR95 we are building uris like file://C:/path/to, that from what I understand are not compatible with the spec.

Also in https://github.com/ami-iit/resolve-robotics-uri-py/pull/11/files#diff-bde2b03789a37955655850a5b32d09d6775334952d2f4fbfe5510b5e005223ffR135 it seems that we are building file://C:/path/to URIs.

diegoferigo · 2024-02-28T10:46:08Z

Thanks a lot! I am not a big fan of erroring on non-existing file:// uri, ma indeed this is coherent with how package:// and model:// work.

Is there any reason not to raise any exception in this case? If I ask a library to resolve an URI, and this URI points to a non-existent resource, I'd expect the library to fail rather than returning silently something that is not usable.

traversaro · 2024-02-28T10:56:19Z

Thanks a lot! I am not a big fan of erroring on non-existing file:// uri, ma indeed this is coherent with how package:// and model:// work.

Is there any reason not to raise any exception in this case? If I ask a library to resolve an URI, and this URI points to a non-existent resource, I'd expect the library to fail rather than returning silently something that is not usable.

I guess it depends from then context or the users. For example, also in C++ filesystem library there is both std::filesystem::canonical (check if the file exists) and std::filesystem::weakly_canonical (does not check if the file exists). Anyhow, for consistency with the other URIs I totally agree that here we should check for existence.

diegoferigo · 2024-02-28T11:49:33Z

I resolved the paths before using them. Now CI on windows fails with the following error:

AssertionError: assert WindowsPath('C:Users/RUNNER~1/AppData/Local/Temp/tmpa2w5uy5e') == WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/tmpa2w5uy5e')

It seems that pathlib resolves the absolute path starting with / on Windows using C:Users/RUNNER~1 when instead tempfile uses C:/Users/runneradmin to create the temporary directory.

diegoferigo · 2024-02-28T12:54:47Z

Partially fixed by using os.pathsep also in tests. I forgot windows uses ; instead of :.

Regardless to this, now the mismatch is the following:

AssertionError: assert WindowsPath('C:Users/runneradmin/AppData/Local/Temp/tmpmxwcnoqt') == WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/tmpmxwcnoqt')

The resolved path is C:Users/... in the first case and C:/Users/ in the other one.

More details in https://stackoverflow.com/a/59840535.

traversaro · 2024-02-28T13:12:27Z

Partially fixed by using os.pathsep also in tests. I forgot windows uses ; instead of :.

Regardless to this, now the mismatch is the following:
AssertionError: assert WindowsPath('C:Users/runneradmin/AppData/Local/Temp/tmpmxwcnoqt') == WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/tmpmxwcnoqt')
The resolved path is C:Users/... in the first case and C:/Users/ in the other one.

More details in https://stackoverflow.com/a/59840535.

I may recall wrong, but that may be the reason why for the file file: scheme I used a custom logic (i.e. just replace file:/ with an empty string:

resolve-robotics-uri-py/src/resolve_robotics_uri_py/resolve_robotics_uri_py.py

Line 52 in c7670f0

model_filenames.append(uri.replace("file:/", ""))

) instead of using more complex logic.

Useful for debugging in CI

diegoferigo · 2024-02-28T13:49:43Z

Now it the behavior should be fixed on all platforms. The path to the found resource is not only guaranteed to be existing, it is also guaranteed to be the resolved path.

Good to go from my side.

traversaro · 2024-02-28T15:16:51Z

I added some tests that I guess should fail with the current version of the logic, while worked fine before.

traversaro · 2024-02-28T15:24:35Z

I added some tests that I guess should fail with the current version of the logic, while worked fine before.

Ok, I have no idea why the CI is not failing as one command is failing, anyhow you can see from the output that the absolute file is not found:

resolve-robotics-uri-py: No file corresponding to URI 'file://C:/Program Files/Git/usr/bin/echo' found

while it should return C:/Program Files/Git/usr/bin/echo (that is the input string).

diegoferigo · 2024-02-28T18:41:52Z

Now the PR should be ready for review. Relevant changes:

Resolving now fails if the file://<absolute_path> path does not exist.
If the URI has no scheme, the pattern file://<string with absolute path> is used by default.
Extended python tests.
Better support of RFC8089.
Returned paths always exists.
Returned path are automatically resolved.

traversaro · 2024-02-28T20:39:34Z

Also the command check is working fine, it was not working as echo is not actual command, I changed the test to use which git and it is working not working, but that is because which git returns the git path without .exe. . PR is good to go for me, do you want to curate the commits?

diegoferigo added 8 commits February 23, 2024 15:58

Update comment in code

b2f6462

Refactor logic and raise exception if file:// URI does not exists

c51ed74

Assume file:// if the URI has no scheme

c4cc68f

Format with black and isort

7bbe6c0

Update command-line logic

ec41525

Handle both absolute and relative paths of file:// scheme

b1c23cb

Update documentation and comments

2b3d57b

Expose the supported environment variables

9efd7a7

diegoferigo mentioned this pull request Feb 23, 2024

Allow relative uri paths also in the file:// scheme #10

Closed

Add new tests

8e9ef4e

diegoferigo force-pushed the refactor branch from 3ea5b5a to 8e9ef4e Compare February 23, 2024 16:26

diegoferigo changed the title ~~Enhance and refactor the resolution logic~~ Enhance and refactor URIs resolving logic Feb 23, 2024

traversaro closed this Feb 24, 2024

traversaro reopened this Feb 24, 2024

traversaro reviewed Feb 24, 2024

View reviewed changes

Enhance search path detection logic from env vars

5253ab6

diegoferigo force-pushed the refactor branch from 20a8ca9 to d822042 Compare February 28, 2024 12:52

diegoferigo force-pushed the refactor branch from e465122 to 898aeee Compare February 28, 2024 13:13

diegoferigo added 4 commits February 28, 2024 14:45

Resolve symlinks and ".." in paths

0f98b04

Configure pytest with the right folder containing tests

7895155

Print output generated by pytest in CI

c761fd8

Useful for debugging in CI

Enhance type hints

90f0285

diegoferigo force-pushed the refactor branch 2 times, most recently from 7a16be3 to 96e41c9 Compare February 28, 2024 13:48

diegoferigo requested a review from traversaro February 28, 2024 13:49

diegoferigo force-pushed the refactor branch 2 times, most recently from fd3ca7d to 1d52fbf Compare February 28, 2024 18:34

traversaro approved these changes Feb 28, 2024

View reviewed changes

diegoferigo force-pushed the refactor branch 2 times, most recently from 21da473 to 7887966 Compare February 29, 2024 07:58

diegoferigo and others added 6 commits February 29, 2024 09:01

Fix tests on windows and macos

929e860

Update file scheme logic

a5ae14a

Update tests

81908de

Add test for resolving fine an existing file without file:/ scheme

a31df11

Move file:/ logic

a52927e

Test file URIs in CI

e6ef710

diegoferigo force-pushed the refactor branch from 7887966 to e6ef710 Compare February 29, 2024 08:37

traversaro merged commit 3ffc433 into ami-iit:main Feb 29, 2024
3 checks passed

diegoferigo deleted the refactor branch February 29, 2024 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance and refactor URIs resolving logic #11

Enhance and refactor URIs resolving logic #11

diegoferigo commented Feb 23, 2024

traversaro commented Feb 24, 2024

traversaro commented Feb 24, 2024

traversaro commented Feb 24, 2024 •

edited

Loading

traversaro Feb 24, 2024

traversaro Feb 24, 2024

diegoferigo Feb 28, 2024

traversaro Feb 28, 2024 •

edited

Loading

diegoferigo Feb 28, 2024

traversaro Feb 28, 2024 •

edited

Loading

diegoferigo Feb 28, 2024

traversaro Feb 28, 2024

traversaro Feb 28, 2024

traversaro Feb 28, 2024

diegoferigo commented Feb 28, 2024 •

edited

Loading

traversaro commented Feb 28, 2024

diegoferigo commented Feb 28, 2024 •

edited

Loading

diegoferigo commented Feb 28, 2024 •

edited

Loading

traversaro commented Feb 28, 2024

diegoferigo commented Feb 28, 2024

traversaro commented Feb 28, 2024

traversaro commented Feb 28, 2024

diegoferigo commented Feb 28, 2024

traversaro commented Feb 28, 2024

Enhance and refactor URIs resolving logic #11

Enhance and refactor URIs resolving logic #11

Conversation

diegoferigo commented Feb 23, 2024

traversaro commented Feb 24, 2024

traversaro commented Feb 24, 2024

traversaro commented Feb 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

traversaro Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

traversaro Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoferigo commented Feb 28, 2024 • edited Loading

traversaro commented Feb 28, 2024

diegoferigo commented Feb 28, 2024 • edited Loading

diegoferigo commented Feb 28, 2024 • edited Loading

traversaro commented Feb 28, 2024

diegoferigo commented Feb 28, 2024

traversaro commented Feb 28, 2024

traversaro commented Feb 28, 2024

diegoferigo commented Feb 28, 2024

traversaro commented Feb 28, 2024

traversaro commented Feb 24, 2024 •

edited

Loading

traversaro Feb 28, 2024 •

edited

Loading

traversaro Feb 28, 2024 •

edited

Loading

diegoferigo commented Feb 28, 2024 •

edited

Loading

diegoferigo commented Feb 28, 2024 •

edited

Loading

diegoferigo commented Feb 28, 2024 •

edited

Loading