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

Use absolute paths in requirements files #816

Merged
merged 9 commits into from
Aug 12, 2022

Conversation

mhsmith
Copy link
Member

@mhsmith mhsmith commented Aug 8, 2022

Fixes #813. I had to use absolute paths in the end, because the Flatpak build moves the requirements file a second time before using it.

In order to get this working with Flatpak, the following additional changes were needed:

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the flatpak support package refactoring problem. Add that one to the list of problems that would have been caught if we actually ran apps as part of CI (See #699).

One minor tweak regarding os.path vs pathlib which I've made on your behalf; and one larger problem with other valid inputs to requirements. I've reworked and extended the test cases to highlight the problem.

# Update paths to be absolute, because flatpak moves the requirements
# file to a different place before using it.
if any(sep in requirement for sep in separators):
requirement = os.path.abspath(self.base_path / requirement)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General preference is to use the pathlib APIs rather than os.path. In this case, this has the added benefit that it resolves symbolic links at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of any benefit of resolving symbolic links here, but I can think of a potential problem. Consider the following sequence of events:

  • The user adds a requirement path with a symlink, then runs briefcase update -d. A requirements.txt file is generated containing the symlink target path.
  • The user changes the target of the symlink.
  • The user runs briefcase build. The existing requirements.txt file is used, with the old symlink target.

The user's mental model is that their requirements list is passed directly to pip: the requirements file is just an implementation detail. So it's reasonable for them to assume that symlinks will be resolved by pip itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a reasonable point... I defaulted to "resolving references is a good thing"; but thinking about it some more, I think you're right that we should preserve the symlink, and let the last tool to use the link do the resolution.

@@ -0,0 +1 @@
Use absolute paths in requirements files.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be 813.misc.rst, not 816.misc.rst - tracking the bug that is closed, not the PR.

@mhsmith
Copy link
Member Author

mhsmith commented Aug 9, 2022

Thanks, I'll look over this tomorrow.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nice work!

@codecov
Copy link

codecov bot commented Aug 12, 2022

Codecov Report

Merging #816 (7d1b3c2) into main (f000a5f) will decrease coverage by 0.02%.
The diff coverage is 88.88%.

Impacted Files Coverage Δ
src/briefcase/commands/create.py 99.33% <85.71%> (-0.33%) ⬇️
src/briefcase/platforms/linux/flatpak.py 100.00% <100.00%> (ø)
src/briefcase/console.py 100.00% <0.00%> (ø)
src/briefcase/integrations/subprocess.py 100.00% <0.00%> (ø)
src/briefcase/integrations/android_sdk.py 99.09% <0.00%> (ø)
src/briefcase/platforms/android/gradle.py 93.20% <0.00%> (+0.20%) ⬆️

@freakboy3742 freakboy3742 merged commit 7a95f4b into beeware:main Aug 12, 2022
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.

Relative paths in requirements files don't work
2 participants