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

Add tempdir() to avoid unwanted package file writing #239

Merged
merged 9 commits into from Aug 4, 2019

Conversation

@RLumSK
Copy link
Contributor

@RLumSK RLumSK commented Aug 2, 2019

Background

Calling create_codemeta(use_filesize = TRUE) (the default) uses pkgbuild::build() to build the package and estimates its file size.

Unfortunately, it does not clean-up after and leaves unwanted files behind in the user's home filespace (here in the parent directory). While doing so, the package violates the CRAN policy:

Packages should not write in the user’s home filespace (including clipboards)

Solution

pkgbuild::build() provides the argument dest_path. This argument can be set to tempdir() to avoid the trouble, which is the solution I propose with my PR.

Further remarks

Besides it might be reasonable to make create_codemeta(use_filesize = FALSE) the default (not part of my PR). If a package contains a lot of code to compile it takes quite some time, and the reason for the waiting time is not immanently obvious.

…ent directory.
@RLumSK
Copy link
Contributor Author

@RLumSK RLumSK commented Aug 2, 2019

The AppVeyor errors are not related to my commit; the VM cannot initialise due to a bad gateway (error 502). Probably is helps to re-run the check.

@cboettig
Copy link
Member

@cboettig cboettig commented Aug 2, 2019

Thanks, completely agree with your points here.

Also, I completely agree with the case for making use_filesize = FALSE the default. Eventually a more reasonable strategy for estimating the file size might be appropriate anyway (e.g. computing the total file size of the source repo would perhaps be sufficient). Like you say, actually building the package seems like an overly aggressive side-effect.

  • Can you go ahead and swap the default, updating the roxygen docs as well?
  • Can you also add yourself as a ctb in DESCRIPTION?
  • Can you also make a mention of these changes in NEWS.md, linking this PR?

🙏 Many thanks!

RLumSK added 5 commits Aug 2, 2019
+ updating the roxygen docs
+ linking this PR
@RLumSK
Copy link
Contributor Author

@RLumSK RLumSK commented Aug 2, 2019

Done. Regarding the file size, my suggestion would be something like this:

sum(file.size(
  list.files(
    ".",
    recursive = TRUE,
    full.names = TRUE,
    all.files = FALSE
  )
)) / 1000

It takes on my computer (five years old) 1.5 ms to compute and it is more realistic than the package size after the package was built, which depends too much on compiler settings. Side effect, if I do not overlook something, it would also remove the dependency to 'pkgbuild'.

@cboettig
Copy link
Member

@cboettig cboettig commented Aug 2, 2019

@RLumSK Thanks, these changes look great. If you'd like, I'd be happy to include the revised file-size calculation and drop the pkgbuild dependency all together.

RLumSK added 3 commits Aug 3, 2019
+ Amend guess_fileSize() in guess_other_metadata.R as proposed #239
+ Update tests
+ Drop dependency to 'pkgbuild'
+ Add NEWS in NEWS.md
+ Amend guess_fileSize() in guess_other_metadata.R as proposed #239
+ Update tests
+ Drop dependency to 'pkgbuild'
+ Update documentation in 'write_codemeta()'
+ Upddate corresponding Rd-files
+ Add NEWS in NEWS.md
…eanup_after_pkg_build
@RLumSK
Copy link
Contributor Author

@RLumSK RLumSK commented Aug 3, 2019

Done. Besides, I kept the newly set default values for use_filesize (FALSE). The file size is a rough estimation only and as written above, it depends on the platform and the compiler settings a package is installed. However, feel free to set it back to TRUE if you like this more.

@cboettig
Copy link
Member

@cboettig cboettig commented Aug 4, 2019

Very nice, thanks for this!

@cboettig cboettig merged commit 63bb55f into ropensci:master Aug 4, 2019
4 checks passed
4 checks passed
codecov/patch 91.7% of diff hit (target 90.5%)
Details
codecov/project Absolute coverage decreased by -0.4% but relative coverage increased by +1.2% compared to 0630dc3
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
cboettig added a commit that referenced this pull request Aug 5, 2019
* Add tempdir() to avoid to writing guess_other_metadata() into the parent directory.

* + swap the default
+ updating the roxygen docs

* + Add myself as a ctb in DESCRIPTION

* + mention of changes in NEWS.md
+ linking this PR

* Address #240 and avoid pre-commit hook without user consent

* Correct link in NEWS.md #240

* Modify the way the file size is calculated.

+ Amend guess_fileSize() in guess_other_metadata.R as proposed #239
+ Update tests
+ Drop dependency to 'pkgbuild'
+ Add NEWS in NEWS.md

* Modify the way the file size is calculated.

+ Amend guess_fileSize() in guess_other_metadata.R as proposed #239
+ Update tests
+ Drop dependency to 'pkgbuild'
+ Update documentation in 'write_codemeta()'
+ Upddate corresponding Rd-files
+ Add NEWS in NEWS.md

* Address #238

+ Add NEWS

* Update tests and add CITATION_ex2 #241
@RLumSK RLumSK deleted the RLumSK:Cleanup_after_pkg_build branch Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.