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

Fallback to cwd as a base path for resources_dir & results_dir #134

Closed

Conversation

obbardc
Copy link
Contributor

@obbardc obbardc commented Jul 7, 2023

Fluster defaults to using the script location as a base path for the resources & results. This works fine for local runs of fluster, when one is just calling fluster from a directory (e.g. ./fluster.py) but when fluster is installed in a read-only system directory (e.g. /usr/lib/python3.11/dist-packages/fluster for Debian) then the resources and results cannot be written unless running as superuser.

Modify this behavior so that if the resources or results directories cannot be found under the script path, use the current working directory as a base path for these directories to allow fluster to be ran from a system installation.

@ylatuya
Copy link
Contributor

ylatuya commented Jul 7, 2023

First of all, thanks for your contribution 🙌

I think fluster should behave the same way whether it's installed or uninstalled to simplify the logic.

For resources, fluster should default to $XDG_DATA_HOME/fluster (or %APPDATA% on Windows), so that regardless of where it's being run, the resources are always downloaded to the same directory and can be reused across executions.

For the results dir, I think we can always default to the current working directory.

Both paths should be configurable through args to override the defaults:
fluster --resources-dir=~/my-fluster/resources/ --results-dir=~/my-fluster/results

@obbardc
Copy link
Contributor Author

obbardc commented Jul 7, 2023

First of all, thanks for your contribution 🙌

I think fluster should behave the same way whether it's installed or uninstalled to simplify the logic.

For resources, fluster should default to $XDG_DATA_HOME/fluster (or %APPDATA% on Windows), so that regardless of where it's being run, the resources are always downloaded to the same directory and can be reused across executions.

For the results dir, I think we can always default to the current working directory.

Both paths should be configurable through args to override the defaults: fluster --resources-dir=~/my-fluster/resources/ --results-dir=~/my-fluster/results

Thanks! The reason for opening this PR was to discuss the best possible outcome ;-).
Would that cause a regression for those who have fluster resources installed locally?

@ylatuya
Copy link
Contributor

ylatuya commented Jul 7, 2023

First of all, thanks for your contribution 🙌
I think fluster should behave the same way whether it's installed or uninstalled to simplify the logic.
For resources, fluster should default to $XDG_DATA_HOME/fluster (or %APPDATA% on Windows), so that regardless of where it's being run, the resources are always downloaded to the same directory and can be reused across executions.
For the results dir, I think we can always default to the current working directory.
Both paths should be configurable through args to override the defaults: fluster --resources-dir=~/my-fluster/resources/ --results-dir=~/my-fluster/results

Thanks! The reason for opening this PR was to discuss the best possible outcome ;-). Would that cause a regression for those who have fluster resources installed locally?

This will be a breaking change, which brings up the need to start doing releases for fluster if the plan is to package for Debian or pypi. I have opened an issue to discuss that here: #135

@obbardc
Copy link
Contributor Author

obbardc commented Sep 20, 2023

Any suggestion on how I should go forward here ? Thanks !

@rgonzalezfluendo
Copy link
Contributor

FYI, You can use next global arguments:

  • --resources (or -r) to set the directory where resources are taken from.
  • --output (or o) to set the directory where test results will be stored.

So, this example should work in a read-only file system:

python ./fluster/fluster.py -r /tmp/resources download AV1-TEST-VECTORS
python ./fluster/fluster.py -o /tmp/out -r /tmp/resources run -ts AV1-TEST-VECTORS

@obbardc
Copy link
Contributor Author

obbardc commented Sep 28, 2023

You can use next global arguments

Right, but what about when fluster is installed in a system path e.g. /usr/bin/fluster ? We should make it pick sensible defaults.

Fluster defaults to using the script location as a base path for the
resources & results. This works fine for local runs of fluster, when
one is just calling fluster from a directory (e.g. ./fluster.py) but
when fluster is installed in a read-only system directory (e.g.
/usr/lib/python3.11/dist-packages/fluster for Debian) then the resources
and results cannot be written unless running as superuser.

Modify this behavior so that if the resources or results directories
cannot be found under the script path, use the current working directory
as a base path for these directories to allow fluster to be ran from a
system installation.

Signed-off-by: Christopher Obbard <[email protected]>
@obbardc obbardc force-pushed the wip/obbardc/fix-install-in-systemdir branch from 234f805 to 8718caa Compare October 3, 2023 08:24
@ylatuya
Copy link
Contributor

ylatuya commented Oct 3, 2023

We should default to $XDG_DATA_HOME/fluster both for the uninstalled and the installed version.

I way to make it forward compatible and avoid breaking existing workflow is by checking in the uninstalled version if any of the subfolders exists, use them instead of the default $XDG_DATA_HOME/fluster but warn people that in future versions the default folder will change and they can use -r or -o.

In summary:

  • If installed: default to $XDG_DATA_HOME/fluster
  • If uninstalled:
    • If resources exists use it and warn, else use $XDG_DATA_HOME/fluster/resources
    • If results exists use it and warn, else use $XDG_DATA_HOME/fluster/results

@obbardc
Copy link
Contributor Author

obbardc commented Oct 3, 2023

@ylatuya sounds good, I can implement this. But I have two questions:

  • How can we tell if fluster is installed? Is it suitable to use something like pkgutil.find_loader or importlib.util.find_spec ?
  • How can we read $XDG_DATA_HOME reliably? Is reading the environment variable enough or should we use e.g https://pypi.org/project/xdg-base-dirs/ ?

@ylatuya
Copy link
Contributor

ylatuya commented Oct 3, 2023

  • How can we tell if fluster is installed? Is it suitable to use something like pkgutil.find_loader or importlib.util.find_spec ?

It's easier to check if it's uninstalled:

is_uninstalled = (Path(__file__).parent / '..' / '.git').exists() and (Path(__file__).parent / '..' / 'fluster.py').exists()

https://pypi.org/project/xdg-base-dirs/ It doesn't seem to be cross-platform, a cross-platform implementation could be: https://github.com/platformdirs/platformdirs. This would be the first external dependency introduced and I think we could avoid it since the implementation would be trivial for this use case.

For the resources cache:

  • Windows: %LOCALAPPDATA%/fluster
  • macOS and linux : $XDG_CACHE_HOME/fluster or $HOME/.cache/fluster if $XDG_CACHE_HOME is not set or empty

For the results directory, thinking it twice with usability in mind, it's maybe better to use the current directory. What do you think @rgonzalezfluendo?

@rgonzalezfluendo
Copy link
Contributor

rgonzalezfluendo commented Oct 3, 2023

For the results directory, thinking it twice with usability in mind, it's maybe better to use the current directory.

Personally, I don't like tools to use the current directory. For instance, getting a fluster folder in my home if a crash...

Also, the current directory can not have write perms.

I prefer to use always tempfile.gettempdir() than the current directory.

UPDATE: python tempdir can be changed with env vars

$ TMPDIR="/tmp/other" python3 -c "from tempfile import gettempdir as f; print(f())"
/tmp/other

@rgonzalezfluendo
Copy link
Contributor

I implemented #145 with all these ideas.

Please let me know your feedback.

@rgonzalezfluendo
Copy link
Contributor

First, I apologize for imposing our solution by creating the PR #145 above yours. We understand this can be frustrating, and there are better ways to operate.

Thank you for opening this topic for discussion. We hope you like the final solution. Please do not hesitate to give feedback or request any changes.

@obbardc
Copy link
Contributor Author

obbardc commented Nov 27, 2023

@rgonzalezfluendo it's fine - this PR was just to start the conversation ;-)

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.

3 participants