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

Make ccache optional (and more) #264

Merged
merged 6 commits into from
May 30, 2020
Merged

Make ccache optional (and more) #264

merged 6 commits into from
May 30, 2020

Conversation

pat-s
Copy link
Member

@pat-s pat-s commented May 28, 2020 •
edited
Loading

fixes #261

This PR also

  • Robustifies update_yml() in a way that it can now handle two [Custom] blocks in a row
  • Removes the custom gfortran v8.2 installation on macOS (in the hope that it gets updated upstream in homebrew cask (PR issued)
  • Adds instructions to vignette "ci-provider" how to add the ccache blocks again (useful for packages with many deps)
  • Adds a tryCatch() in update_yml() that tells the user to update manually if the local template is too different compared to the latest upstream template (i.e. if update_yml() failed entirely)
  • Also removes the custom installation of the 10.13 SKD to mimic CRAN behavior. Not entirely sure if this is a good idea but it bloated the YAML and things might get better over time, which we won't notice otherwise (the 10.15 SDK causes some installation errors when installing from source)

@pat-s pat-s requested a review from krlmlr May 28, 2020 19:30
krlmlr
krlmlr approved these changes May 29, 2020
View reviewed changes
Copy link
Collaborator

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

I still don't understand the yml surgery.

Can/should the ccache update happen in a separate yml workflow?

vignettes/ci-providers.Rmd
The downside is that `ccache` needs to be installed and configured.
This happens in every run, i.e. also in runs in which `ccache` is not used because a package cache already exists.
Installation can take up to 1 min, depending on the platform.
Note that `ccache` won't be used on Windows since only binaries are used on this platform.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it even work?

Copy link
Member Author

Choose a reason for hiding this comment

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

If there would be a compiler, it would.

But this and the burden of installing ccache in the first place is not worth it.

@pat-s @krlmlr
Update vignettes/ci-providers.Rmd
812df73 
Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>
@pat-s
Copy link
Member Author

pat-s commented May 29, 2020

Can/should the ccache update happen in a separate yml workflow?

Interesting. Yeah it could probably be run in a separate workflow, maybe even just once a week.
The created cache could then be used by other builds.

However, the builds always need to have ccache installed in the first place, which is the major delay with respect to runtime.
Downloading the cache does note even take 5 secs.

@pat-s pat-s merged commit f6ad223 into master May 30, 2020
@pat-s pat-s deleted the feature/ccache_optional branch May 30, 2020 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krlmlr krlmlr krlmlr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make the ccache steps optional
2 participants
@pat-s @krlmlr