FawltyDeps fails running on itself #392
Assignees
Labels
Comments
|
FD runs inside the poetry shell/virtualenv (which lives outside the current dir). FD is apparently not able to automatically find this pyenv. It should find this venv according to this logic:
This last fallback seems to fail. Needs more investigation. |
|
I reproduced this locally, and figured out why the fallback to current virtualenv does not work: The problem is in fact that FD does find Python environments, too many, in fact, and none of them are relevant to analyzing FawltyDeps itself. Running There are several virtualenvs found under Possible explanations and potential solutions:
I think I'm currently in favor of the very last option, but I'm eager to hear about perspectives or better solutions that I have missed. What do you think? |
|
I tried to implement the solution (last option) I mentioned above, but ran into annoying difficulties with the way our In summary, things get more complex and unintuitive, when I wanted to make them simpler. So I'm not sure this is the best way forward after all... @mknorps: yesterday, you hinted an alternative solution on Slack, where we add a separate resolver to always look at the current env where FD is running. I guess that is much closer to the first change I proposed above ("Whether or not I started playing around with the code a little, and so far this seems like it's much easier to implement, and it should work with almost no changes to our test suite at all(!) |
jherland
added a commit
that referenced
this issue
Nov 9, 2023
This fixes issue #392 by making sure that in the case where there are only _irrelevant_ venvs located inside the project, we will still find at least _one_ Python environment where we can have some hope of finding relevant dependencies installed. In fact, this is partially a return to our pre v0.7 behavior where we would always (then: only) find packages in sys.path. Since v0.7 we allowed --pyenv to override this, and since v0.12, we also started searching for pyenvs under --pyenv or basepaths given on the command line. The result of this is that by now, the current environment is only consulted in the fallback where no other environment is possibly found. The change here is minimal (due to the preparations in previous commits), and the existing tests are also compatible with this change, except for the self_test Nox target, which can now be simplified to no longer pass a --pyenv option, due to fixing this exact issue. Most of the diff here is simply updating the relevant documentation to reflect and clarify our new policy. In the future, in the unlikely event that including sys.path in the resolver stack introduces any regression, we might consider adding a user-visible option to disable this again.
jherland
added a commit
that referenced
this issue
Nov 9, 2023
This fixes issue #392 by making sure that in the case where there are only _irrelevant_ venvs located inside the project, we will still find at least _one_ Python environment where we can have some hope of finding relevant dependencies installed. In fact, this is partially a return to our pre v0.7 behavior where we would always (then: only) find packages in sys.path. Since v0.7 we allowed --pyenv to override this, and since v0.12, we also started searching for pyenvs under --pyenv or basepaths given on the command line. The result of this is that by now, the current environment is only consulted in the fallback where no other environment is possibly found. The change here is minimal (due to the preparations in previous commits), and the existing tests are also compatible with this change, except for the self_test Nox target, which can now be simplified to no longer pass a --pyenv option, due to fixing this exact issue. Most of the diff here is simply updating the relevant documentation to reflect and clarify our new policy. In the future, in the unlikely event that including sys.path in the resolver stack introduces any regression, we might consider adding a user-visible option to disable this again.
jherland
added a commit
that referenced
this issue
Nov 10, 2023
This fixes issue #392 by making sure that in the case where there are only _irrelevant_ venvs located inside the project, we will still find at least _one_ Python environment where we can have some hope of finding relevant dependencies installed. In fact, this is partially a return to our pre v0.7 behavior where we would always (then: only) find packages in sys.path. Since v0.7 we allowed --pyenv to override this, and since v0.12, we also started searching for pyenvs under --pyenv or basepaths given on the command line. The result of this is that by now, the current environment is only consulted in the fallback where no other environment is possibly found. The change here is minimal (due to the preparations in previous commits), and the existing tests are also compatible with this change, except for the self_test Nox target, which can now be simplified to no longer pass a --pyenv option, due to fixing this exact issue. Most of the diff here is simply updating the relevant documentation to reflect and clarify our new policy. In the future, in the unlikely event that including sys.path in the resolver stack introduces any regression, we might consider adding a user-visible option to disable this again.
jherland
added a commit
that referenced
this issue
Nov 10, 2023
This fixes issue #392 by making sure that in the case where there are only _irrelevant_ venvs located inside the project, we will still find at least _one_ Python environment where we can have some hope of finding relevant dependencies installed. In fact, this is partially a return to our pre v0.7 behavior where we would always (then: only) find packages in sys.path. Since v0.7 we allowed --pyenv to override this, and since v0.12, we also started searching for pyenvs under --pyenv or basepaths given on the command line. The result of this is that by now, the current environment is only consulted in the fallback where no other environment is possibly found. The change here is minimal (due to the preparations in previous commits), and the existing tests are also compatible with this change, except for the self_test Nox target, which can now be simplified to no longer pass a --pyenv option, due to fixing this exact issue. Most of the diff here is simply updating the relevant documentation to reflect and clarify our new policy. In the future, in the unlikely event that including sys.path in the resolver stack introduces any regression, we might consider adding a user-visible option to disable this again.
jherland
added a commit
that referenced
this issue
Nov 10, 2023
This fixes issue #392 by making sure that in the case where there are only _irrelevant_ venvs located inside the project, we will still find at least _one_ Python environment where we can have some hope of finding relevant dependencies installed. In fact, this is partially a return to our pre v0.7 behavior where we would always (then: only) find packages in sys.path. Since v0.7 we allowed --pyenv to override this, and since v0.12, we also started searching for pyenvs under --pyenv or basepaths given on the command line. The result of this is that by now, the current environment is only consulted in the fallback where no other environment is possibly found. The change here is minimal (due to the preparations in previous commits), and the existing tests are also compatible with this change, except for the self_test Nox target, which can now be simplified to no longer pass a --pyenv option, due to fixing this exact issue. Most of the diff here is simply updating the relevant documentation to reflect and clarify our new policy. In the future, in the unlikely event that including sys.path in the resolver stack introduces any regression, we might consider adding a user-visible option to disable this again.
mknorps
pushed a commit
that referenced
this issue
Nov 15, 2023
This fixes issue #392 by making sure that in the case where there are only _irrelevant_ venvs located inside the project, we will still find at least _one_ Python environment where we can have some hope of finding relevant dependencies installed. In fact, this is partially a return to our pre v0.7 behavior where we would always (then: only) find packages in sys.path. Since v0.7 we allowed --pyenv to override this, and since v0.12, we also started searching for pyenvs under --pyenv or basepaths given on the command line. The result of this is that by now, the current environment is only consulted in the fallback where no other environment is possibly found. The change here is minimal (due to the preparations in previous commits), and the existing tests are also compatible with this change, except for the self_test Nox target, which can now be simplified to no longer pass a --pyenv option, due to fixing this exact issue. Most of the diff here is simply updating the relevant documentation to reflect and clarify our new policy. In the future, in the unlikely event that including sys.path in the resolver stack introduces any regression, we might consider adding a user-visible option to disable this again.
mknorps
pushed a commit
that referenced
this issue
Dec 21, 2023
This fixes issue #392 by making sure that in the case where there are only _irrelevant_ venvs located inside the project, we will still find at least _one_ Python environment where we can have some hope of finding relevant dependencies installed. In fact, this is partially a return to our pre v0.7 behavior where we would always (then: only) find packages in sys.path. Since v0.7 we allowed --pyenv to override this, and since v0.12, we also started searching for pyenvs under --pyenv or basepaths given on the command line. The result of this is that by now, the current environment is only consulted in the fallback where no other environment is possibly found. The change here is minimal (due to the preparations in previous commits), and the existing tests are also compatible with this change, except for the self_test Nox target, which can now be simplified to no longer pass a --pyenv option, due to fixing this exact issue. Most of the diff here is simply updating the relevant documentation to reflect and clarify our new policy. In the future, in the unlikely event that including sys.path in the resolver stack introduces any regression, we might consider adding a user-visible option to disable this again.
mknorps
added a commit
that referenced
this issue
Dec 21, 2023
…o latest Test of 'nul' for Windows checks Copilot assisted fix for running subprocess on Windows machine Bump version to 0.13.2 (#389) packages: Expose decision to use current env to resolve packages Until now, LocalPackageResolver has defaulted to only using the current Python environment (aka. sys.path, the one in which FawltyDeps is currently running) in the case where LocalPackageResolver is initialized with zero package dirs. We want to use the current env _in addition to_ other Python environments in some scenarios, so move this decision out of LocalPackageResolver, forward it via setup_resolvers(), and allow the caller of setup_resolvers() to set this flag separately. For now the observed behavior stays the same: Analysis.resolved_deps still looks at whether or not any other Python environment were found when setting use_current_env. packages: Start separating handling of project pyenvs from current env Use separate LocalPackageResolvers instances for Python environments found inside the project vs. the _current_ Python environment in which FawltyDeps is running (aka. sys.path). We're about to make current env a mandatory part of the resolver stack, and it therefore makes sense to start separating it from the other local envs. This will also enable a future refactoring of LocalPackageResolver into two separate classes. Always include current env in the package resolver stack This fixes issue #392 by making sure that in the case where there are only _irrelevant_ venvs located inside the project, we will still find at least _one_ Python environment where we can have some hope of finding relevant dependencies installed. In fact, this is partially a return to our pre v0.7 behavior where we would always (then: only) find packages in sys.path. Since v0.7 we allowed --pyenv to override this, and since v0.12, we also started searching for pyenvs under --pyenv or basepaths given on the command line. The result of this is that by now, the current environment is only consulted in the fallback where no other environment is possibly found. The change here is minimal (due to the preparations in previous commits), and the existing tests are also compatible with this change, except for the self_test Nox target, which can now be simplified to no longer pass a --pyenv option, due to fixing this exact issue. Most of the diff here is simply updating the relevant documentation to reflect and clarify our new policy. In the future, in the unlikely event that including sys.path in the resolver stack introduces any regression, we might consider adding a user-visible option to disable this again. packages: Split SysPathPackageResolver out of LocalPackageResolver Also introduce an InstalledPackageResolver base class to collect the common parts shared between LocalPackageResolver and the new SysPathPackageResolver. Separate check-undeclared, check-unused pre-commit hooks The `check-undeclared` hook runs only on files being committed. Fixes: GH-385 Apply Maria's suggestions from code review Co-authored-by: Maria Knorps <[email protected]> Apply Johan's suggestions from code review Pre-commit hooks: Update README and use --detailed by default Update `README` instructions to reflect the new organization of the pre-commit hooks offered by FawltyDeps. Also call --detailed from the default `entry` as `args` is now reserved for `--code` and `--deps`, respectively. tests/utils.py: Better output when assert_unordered_equivalence fails DirectoryTraversal: Allow attaching multiple data items in .add() Instead of having to call traversal.add(dir, ...) multiple times in order to attach multiple data items to the same 'dir', allow all data items to be passed as *args to the .add() method. Move DirectoryTraversal into its own module In preparation for adding functionality for ignoring files/dirs in the project. dir_traversal: Rename .ignore() to .skip_dir() We're about to introduce more functionality for ignoring files/dirs in a directory structure via .gitignore-style patterns. However, we're already using the term "ignore" to mean something slightly different in DirectoryTraversal: The .ignore() method is used to tell the traversal that some directory should _not_ be traversed. Rename this method to .skip_dir() (and adjust members and comments accordingly). This is more accurate to what this method actually does, and it frees up the "ignore" term to be used in future commits. dir_traversal: Refactor data on each traversal step into its own class Introduce a TraversalStep class to hold the information that is being passed from the DirecotryTraversal object at each step in the traversal process. This is more self-documenting than yielding a os.walk() style tuple, and allows more easily for future extension. test_dir_traversal: Make use of ExpectedTraverseStep self-documenting Each DirectoryTraversalVector encapsulate a test case for DirectoryTraversal, and contains a list of steps that we expect DirectoryTraversal to perform for that test case, aka. an ExpectedTraverseStep. These were initialized by passing four positional arguments to the constructor (most of which are lists in their own right). The result was that it was hard to read an ExpectedTraverseStep initialization and immediate see what information was expected at this traversal step (without constantly cross-referencing with the ExpectedTraverseStep class definition). Instead, make all ExpectedTraverseStep members (except the mandatory .dir) optional (defaulting to empty lists), and use _named_ arguments when intializing them. This makes reading the test cases more straightforward: Instead of ExpectedTraverseStep("a/b/c", ["d"], [], [456, 123]) (meaning that we expect the traversal of "a/b/c" to list a single subdir "d", no filenames, and passing [456, 123] as the attached data) we now get ExpectedTraverseStep("a/b/c", subdirs=["d"], attached=[456, 123]) test_dir_traversal: Document the .add() calls set up in test vectors traverse_project: Stricter typing for attached data Bump version to v0.13.3 ADD discord shield to the README Add message in the readme to join Discord Declare that we support Python v3.12 Progress from Alpha to Beta quality :-) Expand matrix of tested systems by windows and macos, change ubuntu to latest Test of 'nul' for Windows checks Use sys Python to execute FawltyDeps WIP adjust tests to work on windows ADD support for finding virtual environments on Windows Fix more tests Fix rendering of messages for Windows Adjust command line tests to work on Windows WIP fix more tests Expand matrix of tested systems by windows and macos, change ubuntu to latest Test of 'nul' for Windows checks WIP adjust tests to work on windows Fix more tests Fix rendering of messages for Windows Adjust command line tests to work on Windows WIP fix more tests FIX tests for Linux Skip tests for symlinks on Windows - only administrator can create symlinks there FIX after rebase Use available os.devnull object and not a custoom function Adjust NamedTemporaryFile to work with Windows. See documentation https://docs.python.org/3/library/tempfile.html Fix temporary virtual environment creation Adjust testdata in test_types for Windows Fix temporary virtual environment creation Fix last issues with the Windows paths representations Skip tests meant only for Windows in other systems Fix linters and formatters Fixup for type of path representation in tests Adjust integration tests for Windows For testing resolver, use deadline of test dependent on install_deps flag. Withouth that test report incosistency errors - it is much faster to run the test with instaling deps for the second time, as the dependencies are already there. Allow for more than 15 local variables for linter Use sys.executable for upgrading pip and installing dependencies in project_helpers Do not quote requirements for Windows Fix formatting For libraries to be accessable after install, we need to instal it with specific pip binary, not with 'python -m pip instal...' Use deadline only for non install_deps runs of resolver tests Otherwise rely on a github runner timeout mechanism Split venv_script_lines from project_helpers to Windows and Linux-related Fix with Copilot attempt 1 Experiments with real world projects The problem is that the resolver falls back to the global resolver in tests, maybe run on the fresh runner instead of a local PC will help. Parse requirements for windows real project tests Enclose requirements in double quotes for Windows Test with a single quote marker
mknorps
added a commit
that referenced
this issue
Dec 21, 2023
…o latest Test of 'nul' for Windows checks Copilot assisted fix for running subprocess on Windows machine Bump version to 0.13.2 (#389) packages: Expose decision to use current env to resolve packages Until now, LocalPackageResolver has defaulted to only using the current Python environment (aka. sys.path, the one in which FawltyDeps is currently running) in the case where LocalPackageResolver is initialized with zero package dirs. We want to use the current env _in addition to_ other Python environments in some scenarios, so move this decision out of LocalPackageResolver, forward it via setup_resolvers(), and allow the caller of setup_resolvers() to set this flag separately. For now the observed behavior stays the same: Analysis.resolved_deps still looks at whether or not any other Python environment were found when setting use_current_env. packages: Start separating handling of project pyenvs from current env Use separate LocalPackageResolvers instances for Python environments found inside the project vs. the _current_ Python environment in which FawltyDeps is running (aka. sys.path). We're about to make current env a mandatory part of the resolver stack, and it therefore makes sense to start separating it from the other local envs. This will also enable a future refactoring of LocalPackageResolver into two separate classes. Always include current env in the package resolver stack This fixes issue #392 by making sure that in the case where there are only _irrelevant_ venvs located inside the project, we will still find at least _one_ Python environment where we can have some hope of finding relevant dependencies installed. In fact, this is partially a return to our pre v0.7 behavior where we would always (then: only) find packages in sys.path. Since v0.7 we allowed --pyenv to override this, and since v0.12, we also started searching for pyenvs under --pyenv or basepaths given on the command line. The result of this is that by now, the current environment is only consulted in the fallback where no other environment is possibly found. The change here is minimal (due to the preparations in previous commits), and the existing tests are also compatible with this change, except for the self_test Nox target, which can now be simplified to no longer pass a --pyenv option, due to fixing this exact issue. Most of the diff here is simply updating the relevant documentation to reflect and clarify our new policy. In the future, in the unlikely event that including sys.path in the resolver stack introduces any regression, we might consider adding a user-visible option to disable this again. packages: Split SysPathPackageResolver out of LocalPackageResolver Also introduce an InstalledPackageResolver base class to collect the common parts shared between LocalPackageResolver and the new SysPathPackageResolver. Separate check-undeclared, check-unused pre-commit hooks The `check-undeclared` hook runs only on files being committed. Fixes: GH-385 Apply Maria's suggestions from code review Co-authored-by: Maria Knorps <[email protected]> Apply Johan's suggestions from code review Pre-commit hooks: Update README and use --detailed by default Update `README` instructions to reflect the new organization of the pre-commit hooks offered by FawltyDeps. Also call --detailed from the default `entry` as `args` is now reserved for `--code` and `--deps`, respectively. tests/utils.py: Better output when assert_unordered_equivalence fails DirectoryTraversal: Allow attaching multiple data items in .add() Instead of having to call traversal.add(dir, ...) multiple times in order to attach multiple data items to the same 'dir', allow all data items to be passed as *args to the .add() method. Move DirectoryTraversal into its own module In preparation for adding functionality for ignoring files/dirs in the project. dir_traversal: Rename .ignore() to .skip_dir() We're about to introduce more functionality for ignoring files/dirs in a directory structure via .gitignore-style patterns. However, we're already using the term "ignore" to mean something slightly different in DirectoryTraversal: The .ignore() method is used to tell the traversal that some directory should _not_ be traversed. Rename this method to .skip_dir() (and adjust members and comments accordingly). This is more accurate to what this method actually does, and it frees up the "ignore" term to be used in future commits. dir_traversal: Refactor data on each traversal step into its own class Introduce a TraversalStep class to hold the information that is being passed from the DirecotryTraversal object at each step in the traversal process. This is more self-documenting than yielding a os.walk() style tuple, and allows more easily for future extension. test_dir_traversal: Make use of ExpectedTraverseStep self-documenting Each DirectoryTraversalVector encapsulate a test case for DirectoryTraversal, and contains a list of steps that we expect DirectoryTraversal to perform for that test case, aka. an ExpectedTraverseStep. These were initialized by passing four positional arguments to the constructor (most of which are lists in their own right). The result was that it was hard to read an ExpectedTraverseStep initialization and immediate see what information was expected at this traversal step (without constantly cross-referencing with the ExpectedTraverseStep class definition). Instead, make all ExpectedTraverseStep members (except the mandatory .dir) optional (defaulting to empty lists), and use _named_ arguments when intializing them. This makes reading the test cases more straightforward: Instead of ExpectedTraverseStep("a/b/c", ["d"], [], [456, 123]) (meaning that we expect the traversal of "a/b/c" to list a single subdir "d", no filenames, and passing [456, 123] as the attached data) we now get ExpectedTraverseStep("a/b/c", subdirs=["d"], attached=[456, 123]) test_dir_traversal: Document the .add() calls set up in test vectors traverse_project: Stricter typing for attached data Bump version to v0.13.3 ADD discord shield to the README Add message in the readme to join Discord Declare that we support Python v3.12 Progress from Alpha to Beta quality :-) Expand matrix of tested systems by windows and macos, change ubuntu to latest Test of 'nul' for Windows checks Use sys Python to execute FawltyDeps WIP adjust tests to work on windows ADD support for finding virtual environments on Windows Fix more tests Fix rendering of messages for Windows Adjust command line tests to work on Windows WIP fix more tests Expand matrix of tested systems by windows and macos, change ubuntu to latest Test of 'nul' for Windows checks WIP adjust tests to work on windows Fix more tests Fix rendering of messages for Windows Adjust command line tests to work on Windows WIP fix more tests FIX tests for Linux Skip tests for symlinks on Windows - only administrator can create symlinks there FIX after rebase Use available os.devnull object and not a custoom function Adjust NamedTemporaryFile to work with Windows. See documentation https://docs.python.org/3/library/tempfile.html Fix temporary virtual environment creation Adjust testdata in test_types for Windows Fix temporary virtual environment creation Fix last issues with the Windows paths representations Skip tests meant only for Windows in other systems Fix linters and formatters Fixup for type of path representation in tests Adjust integration tests for Windows For testing resolver, use deadline of test dependent on install_deps flag. Withouth that test report incosistency errors - it is much faster to run the test with instaling deps for the second time, as the dependencies are already there. Allow for more than 15 local variables for linter Use sys.executable for upgrading pip and installing dependencies in project_helpers Do not quote requirements for Windows Fix formatting For libraries to be accessable after install, we need to instal it with specific pip binary, not with 'python -m pip instal...' Use deadline only for non install_deps runs of resolver tests Otherwise rely on a github runner timeout mechanism Split venv_script_lines from project_helpers to Windows and Linux-related Fix with Copilot attempt 1 Experiments with real world projects The problem is that the resolver falls back to the global resolver in tests, maybe run on the fresh runner instead of a local PC will help. Parse requirements for windows real project tests Enclose requirements in double quotes for Windows Test with a single quote marker
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
When I install from scratch a developer's environment and run
fawltydepsin it I get following message:To Reproduce
Run fawltydeps from
poetry shell.Expected behavior
FawltyDeps does not report any missing or unused dependencies
Environment
The text was updated successfully, but these errors were encountered: