Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[path_provider_linux] Using TMPDIR env as a primary temporary path #4218

Merged
merged 8 commits into from
Sep 14, 2021

Conversation

proninyaroslav
Copy link
Contributor

@proninyaroslav proninyaroslav commented Aug 5, 2021

TMPDIR is a standard variable on UNIX/Linux systems, and is often used in containers such as Flatpak to redirect to a temporary folder inside a sandbox. This allows not to make hard bindings to the /tmp directory

Fixes flutter/flutter#87742

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

TMPDIR is a standard variable on UNIX/Linux systems, and is often used in containers such as Flatpak to redirect to a temporary folder inside a sandbox. This allows not to make hard bindings to the /tmp directory
@stuartmorgan
Copy link
Contributor

stuartmorgan commented Aug 5, 2021

Thanks for the submission! This will need the remaining items in the checklist (other than doc comments) addressed before it moves forward with review. If you have any questions about completing those steps that aren’t addressed in the linked documentation, please let me know.

@proninyaroslav
Copy link
Contributor Author

@stuartmorgan
I have questions about this:

I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
I updated CHANGELOG.md to add a description of the change.

Do I have to make a version bump in this case?

@proninyaroslav
Copy link
Contributor Author

@stuartmorgan
About tests: the problem is that the Platform class from the dart:io library is not mockable, so I can't insert the required environment variable value inside the test (Platform.environment). The solution would be to use the mockable version of Platform, some plugins use this https://pub.dev/packages/platform

@stuartmorgan
Copy link
Contributor

stuartmorgan commented Aug 9, 2021

I have questions about this:

I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
I updated CHANGELOG.md to add a description of the change.

Do I have to make a version bump in this case?

The section you are quoting links to an explanation of the versioning policy for flutter/plugins. Could you elaborate on what your are seeing there that suggests you would not need to so that I can clarify it?

About tests: the problem is that the Platform class from the dart:io library is not mockable, so I can't insert the required environment variable value inside the test (Platform.environment). The solution would be to use the mockable version of Platform, some plugins use this https://pub.dev/packages/platform

Since all you need is the environment, a much simpler solution at this point would just be to make PathProviderLinux take an injectable-by-tests environment map that would replace the call to Platform.environment. We generally try to minimize package dependencies in the plugins here.

@proninyaroslav
Copy link
Contributor Author

@stuartmorgan
I added tests and also bumped the plugin version to 2.1.0 as I believe the API changes are backwards compatible and are not just a patch.

@proninyaroslav
Copy link
Contributor Author

@stuartmorgan
All comments have been resolved.

@proninyaroslav
Copy link
Contributor Author

@stuartmorgan
Can you review my changes again?

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Looking again, the tests would be much clearer as three distinct tests rather than one test that alternates between changing the environment and then making new assertions. Rather than do another round of review, I just made those change in the PR.

LGTM with that. Thanks!

@stuartmorgan stuartmorgan added waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. and removed needs tests labels Sep 3, 2021
@stuartmorgan
Copy link
Contributor

Oops, this got caught by the Windows bot change so it was never auto-landed. Landing manually, since this can't affect Windows.

@stuartmorgan stuartmorgan removed the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Sep 14, 2021
@stuartmorgan stuartmorgan merged commit 2076d1d into flutter:master Sep 14, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 14, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 14, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 14, 2021
amantoux pushed a commit to amantoux/plugins that referenced this pull request Sep 27, 2021
…lutter#4218)

TMPDIR is a standard variable on UNIX/Linux systems, and is often used in containers such as Flatpak to redirect to a temporary folder inside a sandbox. This allows not to make hard bindings to the /tmp directory

Fixes flutter/flutter#87742
KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021
…lutter#4218)

TMPDIR is a standard variable on UNIX/Linux systems, and is often used in containers such as Flatpak to redirect to a temporary folder inside a sandbox. This allows not to make hard bindings to the /tmp directory

Fixes flutter/flutter#87742
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[path_provider] Using TMPDIR env as a primary temporary path
2 participants