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

Fix erroneous relative path resolution #726

Merged
merged 1 commit into from
Feb 13, 2025
Merged

Fix erroneous relative path resolution #726

merged 1 commit into from
Feb 13, 2025

Conversation

ivanvig
Copy link
Contributor

@ivanvig ivanvig commented Jan 24, 2025

Currently FuseSoC has two problems related to relative path resolution,

The more severe one occurs when the fusesoc.conf file is in the current directory, in that case the code

files_read = self._cp.read(config_files)

returns only the file name, then in the function _resolve_path_from_cfg when os.path.dirname is called, an empty string is returned and when joined to form the final path we get a broken result

A second issue that I found is when referencing to a relative path starting with a dot, for example ./build_root that dot stays in the path producing a result like /abs/path/to/./build_root which although is a valid path, fails path comparisons, like the ones that are done for ignored_dirs

This PR includes tests for both cases and fixes them.

@ivanvig
Copy link
Contributor Author

ivanvig commented Feb 7, 2025

@olofk could you take a look at this PR?

@olofk
Copy link
Owner

olofk commented Feb 10, 2025

This looks like a nice cleanup of the paths, but the tests fail on macos. Could you take a look at the logs and see if you can figure out what potentially needs to be fixed?

Fixed wrong path resolution when fusesoc.conf file is in current dir,
also improved path resolution to avoid erros when comparing paths
@ivanvig
Copy link
Contributor Author

ivanvig commented Feb 10, 2025

It seems that realpath was adding a parent directory in macos which made the assertions fail, changed for normpath which does just what I wanted. I guess it should be fixed now, if it fails again I will get a macos docker image to run the tests.

@olofk olofk merged commit 4a799ad into olofk:main Feb 13, 2025
8 of 12 checks passed
@olofk
Copy link
Owner

olofk commented Feb 13, 2025

Tests are passing now, so let's hope it stays that way. Thank you for your contributions. Picked and pushed!

@olofk
Copy link
Owner

olofk commented Feb 13, 2025

Huh! Now it failed again? F-in inconstent Python. If you have some bandwidth to look at this, it would be highly appreciated.

@ivanvig
Copy link
Contributor Author

ivanvig commented Feb 13, 2025

Yep, I will look into it, I enabled the CI in my fork so I will test it there. It seems to be a problem related to how macos handles the /var directory, kinda annoying but it must be fixed.

@ivanvig
Copy link
Contributor Author

ivanvig commented Feb 14, 2025

Fixed in #731

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

Successfully merging this pull request may close these issues.

2 participants