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 use_makefile() and use_jenkinsfile(). #501

Merged
merged 11 commits into from
May 3, 2019

Conversation

ryapric
Copy link
Contributor

@ryapric ryapric commented Nov 24, 2018

Added templates & calling functions to place Makefiles and Jenkinsfiles at project root.

R CMD check results, which I ran using the --as-cran flag:

── R CMD check results ───────────────────────────────── usethis 1.4.0.9000 ────
Duration: 33.6s

0 errors ✔ | 0 warnings ✔ | 0 notes ✔

R CMD check succeeded

Thanks!

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

I think these should be use_make() and use_jenkins()

@jimhester do these default make steps seem reasonable to you?

@ryapric
Copy link
Contributor Author

ryapric commented Nov 26, 2018

On further review, R CMD INSTALL won't install dependencies listed in the DESCRIPTION (frustratingly). So, in the Makefile, adding another target called (for example) devtools_install could call devtools::install() via Rscript could solve the issue. Depending on devtools being installed, of course.

Thoughts?

@ryapric
Copy link
Contributor Author

ryapric commented Dec 1, 2018

I've updated with your suggestions, and their implied changes.

  • Functions are now use_<make:jenkins>()

  • Makefile uses devtools::install() as part of the default targets (first checking if devtools itself is installed), which handles dependencies, etc. gracefully. A second make install target, install_src, instead calls R CMD INSTALL for those that would rather do it that way.

Note that the AppVeyor build on Windows seems to have failed because of comparing string literal paths (failed because of path separator discrepancy), instead of resolving the path separators. I am not certain if this is because of the new tests I added, because the failures are thrown via calls to the helper functions:

  1. Failure: edit_r_XXX('user') ensures the file exists (@test-edit.R#40) 
  2. Failure: edit_r_profile() respects R_PROFILE_USER (@test-edit.R#56)

All tests, build steps, etc. pass on my end (Linux), as do the Travis builds.

@hadley
Copy link
Member

hadley commented Dec 2, 2018

Can you please merge the latest from master? That should fix the appveyor issues.

@ryapric
Copy link
Contributor Author

ryapric commented Dec 2, 2018

Holy cow, that's a lot of commits since last week haha. Updated, thanks!

@hadley hadley requested a review from jimhester December 2, 2018 20:20
Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

@jimhester can you please take a look at the specific make steps?

R/jenkins.R Outdated Show resolved Hide resolved
R/make.R Outdated Show resolved Hide resolved
Copy link
Member

@jimhester jimhester left a comment

Choose a reason for hiding this comment

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

I added some suggestions I think would improve the Makefile rules.

inst/templates/Makefile Outdated Show resolved Hide resolved
inst/templates/Makefile Outdated Show resolved Hide resolved
inst/templates/Makefile Outdated Show resolved Hide resolved
inst/templates/Makefile Outdated Show resolved Hide resolved
inst/templates/Makefile Outdated Show resolved Hide resolved
inst/templates/Makefile Outdated Show resolved Hide resolved
all: build check install clean

build:
$(R) build .
Copy link
Member

Choose a reason for hiding this comment

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

I guess we should think about if it would be better to build the tarball in the current working directory or the parent... I don't have a strong preference either way...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, good point. I feel that if it's built in the current directory (hopefully package top-level), then that would be easier to control for by the end-user (via .gitignore etc), and more portable besides. If a user stores all their package code in folders where the parent of each is write-restricted or something else weird, then the build step would fail. Enforcing focus on the current-and-below path should be much more manageable, by everyone. I'm happy to hear thoughts, though.

@jennybc
Copy link
Member

jennybc commented Mar 28, 2019

@ryapric Can you give me a high-level overview of the objective here? I get that this creates a Makefile but to what purpose? What is the workflow facilitated by the added files?

Context: I'm readying for release and am trying to get our PRs / issues under control for the first time in a while. I'm triaging into "for current release", "backlog", "will not pursue at this time".

@jennybc jennybc added this to the Backlog milestone Mar 28, 2019
@ryapric
Copy link
Contributor Author

ryapric commented Mar 29, 2019

Hi @jennybc, do I win the award for longest-stale PR yet?

To address your question, adding a well-written Makefile to someone's R package workflow makes their build/test/etc. steps move entirely with their code. RStudio can't always be around to facilitate those steps, and not everyone uses Travis, etc. where the steps are already configured. Exposing targets like make test to any build system of the developer's choosing (e.g. Jenkins, etc.) facilitates portable, readable CI/CD.

I'd like to take an hour or so to address the great feedback that @jimhester and @hadley offered above, but if you want to keep this under the Backlog milestone, that's fine.

@hadley
Copy link
Member

hadley commented Mar 29, 2019

@ryapric you PR has to be at least 3 years old to even be in the running for that award 🤣

@jennybc
Copy link
Member

jennybc commented Mar 29, 2019

I'm neutral on this and defer to @jimhester and @hadley.

If they're on board, I'm happy to rescue from the backlog if you want to finish this off.

@ryapric
Copy link
Contributor Author

ryapric commented Mar 29, 2019

Cool, entirely up to you all, then. I've pushed all the suggested changes, CI builds are running at the time of this posting. Waiting for a comment reply above from @jimhester, but he said he doesn't have strong opinions on the matter (which directory the build step is in).

@jennybc jennybc modified the milestones: Backlog, v1.5.0 Mar 29, 2019
@jennybc jennybc removed this from the Backlog milestone Apr 10, 2019
@jimhester
Copy link
Member

I think the Makefile part of this looks good now

Copy link
Member

@jennybc jennybc left a comment

Choose a reason for hiding this comment

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

Make these small changes and I'll merge!

R/jenkins.R Outdated Show resolved Hide resolved
R/jenkins.R Outdated Show resolved Hide resolved
@ryapric
Copy link
Contributor Author

ryapric commented May 3, 2019

Great catch, thank you!

@jennybc
Copy link
Member

jennybc commented May 3, 2019

You're going to kill me but I just realized we still need a NEWS bullet, since this adds 2 new exported functions.

Can you please add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber).

@ryapric
Copy link
Contributor Author

ryapric commented May 3, 2019

Sure, do you want it under the current 1.5.0 heading, or do you want me to bump it to a dev version since 1.5.0 already went out?

@jennybc
Copy link
Member

jennybc commented May 3, 2019

There should be a dev heading but I guess there's not. Yes please add such a heading and I'll sort it out from there, after merge.

@jennybc
Copy link
Member

jennybc commented May 3, 2019

Nevermind. There is such a heading and you need to merge in master.

@ryapric
Copy link
Contributor Author

ryapric commented May 3, 2019

I see it now, sorry! Pushed.

@jennybc jennybc merged commit 815deba into r-lib:master May 3, 2019
@ryapric ryapric mentioned this pull request Jul 6, 2019
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.

None yet

4 participants