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.

Sign up for GitHub

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

Jump to bottom

Reduce dependencies needed to build book #546

Closed
Robinlovelace opened this issue Jun 13, 2020 · 17 comments
Closed

Reduce dependencies needed to build book #546

Robinlovelace opened this issue Jun 13, 2020 · 17 comments

Comments

@Robinlovelace
Copy link
Collaborator

Robinlovelace commented Jun 13, 2020 •
edited

With 50+ dependencies, some of which are used only in one chapter in code chunks that do not run, I think there is an incentive to move some of the Imports listed here to Suggests, to reduce build times: https://github.com/geocompr/geocompkg/blob/master/DESCRIPTION
Robinlovelace added a commit that referenced this issue Jun 13, 2020
@Robinlovelace
Do not build c14 for #546
da225bf
Robinlovelace added a commit that referenced this issue Jun 13, 2020
@Robinlovelace
eval=FALSE for parts of c11 for #546
68d6b7b
Robinlovelace referenced this issue in geocompx/geocompkg Jun 13, 2020
@Robinlovelace
Move Imports to Suggests for robinlovelace/geocompr#546
6139842
@Robinlovelace
Copy link
Collaborator Author

One question for @Nowosad, any idea why GH actions is still spending ages installing mapdeck and other unnused packages, even after I demoted them to Suggests here https://github.com/geocompr/geocompkg/pull/16/files ?

On https://github.com/Robinlovelace/geocompr/pull/549 there is this:

Remotes: 
    # geocompr/geocompkg, # old
    geocompr/geocompkg@reduce-deps, # new

But mapdeck and friends are still being installed, pushing build times to over an hour!:

https://github.com/Robinlovelace/geocompr/runs/768446869#step:10:2712

Robinlovelace added a commit that referenced this issue Jun 13, 2020
@Robinlovelace
Do not install Suggests for #546
f6c6656
@Robinlovelace
Copy link
Collaborator Author

Update: I think the above commit is the answer and hopefully the solution.

@Robinlovelace
Copy link
Collaborator Author

Still seems to insist on installing mapdeck: https://github.com/Robinlovelace/geocompr/runs/768582519#step:10:256

@Robinlovelace
Copy link
Collaborator Author

OK just realised: tmap itself indirectly depends on mapdeck, plus it's a useful package. Removing pROC and mlr and a few others will be worthwhile though. tmap depends on leafem which in turn depends on mapdeck it seems: https://cran.r-project.org/web/packages/leafem/index.html

@Robinlovelace Robinlovelace mentioned this issue Jun 13, 2020
Closed
Robinlovelace added a commit that referenced this issue Jun 13, 2020
@Robinlovelace
Do not build c14 for #546
f144d10
Robinlovelace added a commit that referenced this issue Jun 13, 2020
@Robinlovelace
eval=FALSE for parts of c11 for #546
c9279f0
Robinlovelace added a commit that referenced this issue Jun 13, 2020
@Robinlovelace
Do not install Suggests for #546
e48d810
@Robinlovelace
Copy link
Collaborator Author

Closing for now as it's done. However, the book still has loads of dependencies including mapdeck, surprisingly indirectly required by tmap, and there is an unmerged pr in the geocompkg repo. Can you take a look @Nowosad ? I have been a bit brutal in removing dependencies and setting eval=FALSE in C9 C11 and C14. Apologies @jannes-m . I hope to get more (if not all) chunks evaluating in those chapters in future versions but that's for another day and another issue.

@Nowosad
Copy link
Member

Nowosad commented Jun 19, 2020

@Robinlovelace there are two PR in the geocompkg repo. Which is the good one?

@Robinlovelace
Copy link
Collaborator Author

Robinlovelace commented Jun 19, 2020 •
edited

Good point, only one good PR now. The purge-suggests branch was just a test.

@Robinlovelace
Copy link
Collaborator Author

And as a related point think about dependencies led to this: r-spatial/leafem#21

@Nowosad
Copy link
Member

Nowosad commented Jun 19, 2020

Neat

Nowosad referenced this issue in geocompx/geocompkg Jun 19, 2020
@Nowosad
Merge pull request #16 from geocompr/reduce-deps
4cdd48a 
Move Imports to Suggests for robinlovelace/geocompr#546
@Nowosad Nowosad reopened this Jun 19, 2020
@Nowosad
Copy link
Member

Nowosad commented Jun 19, 2020

@Robinlovelace please look at https://github.com/Robinlovelace/geocompr/runs/788038868?check_suite_focus=true - "Failed with error: 'there is no package called 'data.table''"

@Robinlovelace
Copy link
Collaborator Author

This is the error I see:

The OSRM server returned an error:
##[error]Error: lexical error: invalid char in json text.
                                       <!DOCTYPE HTML PUBLIC "-//IETF/
                     (right here) ------^

@Robinlovelace
Copy link
Collaborator Author

https://github.com/Robinlovelace/geocompr/runs/788038868?check_suite_focus=true#step:10:7706

@Robinlovelace
Copy link
Collaborator Author

Correction, I see what you mean now: https://github.com/Robinlovelace/geocompr/runs/788038868?check_suite_focus=true#step:10:7694

@Robinlovelace
Copy link
Collaborator Author

I realise now: just adding quietly = TRUE fixes it. That is strange!

ropensci/stplanr#408

@Robinlovelace
Copy link
Collaborator Author

Will submit to CRAN asap.

@Robinlovelace
Copy link
Collaborator Author

Heads-up @Nowosad: stplanr 0.6.1 is now on CRAN and the dependency on data.table should have been removed.

@Robinlovelace Robinlovelace mentioned this issue Jun 23, 2020
Closed
@Robinlovelace
Copy link
Collaborator Author

Fixed, finally!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Robinlovelace @Nowosad