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

Record commit at package installation #3390

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

infotroph
Copy link
Member

Description

  • Adds a build_hash column to the pecan_version() output, reporting for each package the commit it was installed from and whether there were any uncommitted changes at install time.
  • pecan_version() output now has class "pecan_version_report", and a custom print method that summarizes versions and installation sources more compactly
  • To enable commit recording, all packages gain a new internal variable .build_hash, set at build time to the value of environment variable PECAN_GIT_REV (or to "unknown" if that is not set).
  • make install now exports PECAN_GIT_REV to each package build, including adding +mod to the commit hash for packages installed with uncommitted changes in the working tree.
  • pecan_version() looks the commit up from one of two places:
    • if the DESCRIPTION has a RemoteSha: field, we use that. This is true for packages installed from github using devtools or remotes or pak, or from R-universe using any method.
    • otherwise, we use the value of .build_hash from inside the package namespace. This is set for packages installed locally with make install.
    • Note that for packages installed locally without using Make (e.g. devtools::install("base/utils"), .build_hash will be used but will be set to "unknown" unless you manually set PECAN_GIT_REV to match the current commit before starting the build process.

Motivation and Context

Output from the existing version:

PEcAn.all::pecan_version() |> head(7)
                 package v1.8.0  installed
3              PEcAn.all  1.8.0 1.8.0.9000
4        PEcAn.allometry  1.7.3 1.7.3.9000
5      PEcAn.assim.batch  1.8.0 1.8.0.9000
7           PEcAn.BASGRA  1.8.0 1.8.0.9000
8        PEcAn.benchmark  1.7.3 1.7.3.9000
9           PEcAn.BIOCRO  1.7.3 1.7.3.9000
10           PEcAn.CABLE  1.7.3       <NA>
                                                        source
3                https://pecanproject.r-universe.dev (R 4.4.1)
4         local (/Users/chrisb/clones/pecan/modules/allometry)
5       local (/Users/chrisb/clones/pecan/modules/assim.batch)
7                                                        local
8         local (/Users/chrisb/clones/pecan/modules/benchmark)
9                https://pecanproject.r-universe.dev (R 4.4.1)
10                                                        <NA>

Output with these changes:

PEcAn.all::pecan_version() |> head(7)
 package               v1.8.0 installed               source              
 PEcAn.all             1.8.0  1.8.0.9000 (f78fd1faa2) local (/Users/chr...
 PEcAn.allometry       1.7.3  1.7.3.9000 (059a93d578) local (/Users/chr...
 PEcAn.assim.batch     1.8.0  1.8.0.9000 (059a93+mod) local (/Users/chr...
 PEcAn.BASGRA          1.8.0  1.8.0.9000 (unknown)    local
 PEcAn.benchmark       1.7.3  1.7.3.9000 (8248461346) local (/Users/chr...
 PEcAn.BIOCRO          1.7.3  1.7.3.9000 (08998d2c44) https://pecanproj...
 PEcAn.CABLE           1.7.3  NA                      NA                  

Information visible here:

  • The PEcAn packages in this R library were installed from at least 4 different commits: f78fd, 059a9, 82484, 08998
  • PEcAn.assim.batch was last installed with uncommitted changes ("+mod") on top of commit 059a93d
  • PEcAn.BASGRA was built without recording its hash (in this case by me running devtools::install without exporting PECAN_GIT_REV first, but this output can't distinguish between that and other manual build methods). If I had used make install, the hash and source path would have been recorded here.
  • PEcAn.BIOCRO is installed from R-Universe not from my on-disk repo; its build hash is taken from the RemoteSha field that R-Universe adds to DESCRIPTION at build time.
  • I do not have PEcAn.CABLE installed at all.

The truncation of source (local (/Users/chr...)and the concatenation of version and build hash into one string are only done during printing.
To see their full values, extract the column or coerce the output to a dataframe:

PEcAn.all::pecan_version() |> _$source |> head(7)
[1] "local (/Users/chrisb/clones/pecan/base/all)"           
[2] "local (/Users/chrisb/clones/pecan/modules/allometry)"  
[3] "local (/Users/chrisb/clones/pecan/modules/assim.batch)"
[4] "local"      
[5] "local (/Users/chrisb/clones/pecan/modules/benchmark)"  
[6] "https://pecanproject.r-universe.dev (R 4.4.1)"         
[7] NA

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • My name is in the list of CITATION.cff
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

On my machine this drops from ~3-4 sec for first call and ~2 after,
to ~2 sec for first call and ~1 after.
No longer feels "too slow" to me.
Probably not a big difference, but felt to me like it make the Make output a little easier to interpret
- use args provided at print time
- default to no rownames and left alignment
- truncate more
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant