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

Update ghactions-core.yml #300

Merged
merged 6 commits into from
Jun 25, 2021
Merged

Update ghactions-core.yml #300

merged 6 commits into from
Jun 25, 2021

Conversation

Robinlovelace
Copy link
Member

No description provided.

@Robinlovelace
Copy link
Member Author

Heads-up @pat-s the thinking here is that this will solve countless hours searching for system deps. Not sure I've implemented it in the best possible way but I think it should work out of the box now with packages that have external deps like sf. Belated solution to an issue I opened way back on the topic I think. Thanks for a great package!

@pat-s
Copy link
Member

pat-s commented Dec 20, 2020

Thanks Robin.

I am aware of this function since quite some time.
It helps in certain situations but is not a general solution.
Why?

It only queries the entrys of the root package via its DESCRIPTION file.
Many package authors do not specify their system libs requirements and usually the ones who do have no issues listing their reqs also within the CI YAML file.

It only works realiably on Linux and usually actions needs to be taken to also implement it on macOS.

Next, many system libs requirements origin from package dependencies which won't be covered by this call.

And finally adding this piece of code may suggest that system libs are taken care of by {tic} in all cases (which is not true) and hence may cause confusion.
Due to the points outlined above I prefer being clear about requiring manual action for system libs support.

Let me know if I missed any pros/cons.

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Dec 21, 2020

Let me know if I missed any pros/cons.

Thanks for the reply, covers many of the cons. I think these could be outweighed by the pros if implemented sensibly:

  • Popular packages such as sf and tmap will work on the cheapest distro out of the box with usethis::use_tic()
  • The Linux CIs that may benefit most from this are 10 times cheaper per minute than MAC

image

  • If solutions are only adopted after they run perfectly progress would be slow

The point about costs has affected me: partly because usethis defaults to macOS for PRs we ran out of credits on the ITSLeeds team and are trying to switch to Linux based builds, possibly with help from tic.

I see your point about confusion but given the fact that the whole CI process is confusing to newcomers, the extra burden of another line is seems negligible. But worth thinking about ways to mitigate the negatives.

Some suggestions:

  • Add an argument like use_sysreqs = FALSE to enable the user to choose whether or not the partial solution adding system requirements is used, setting it as FALSE by default would I think mitigate all the negatives you outline above.
  • Add good comments in the .yaml where the line is added and (if possible) print the output to demystify what it does

Thoughts? Happy to help out with the implementation of the idea if that would help and as I say don't see this PR as ready - just hoping to get a conversation going. Hoping words can lead to positive change (vested interest: I hope to use this functionality if it becomes available)!

@pat-s
Copy link
Member

pat-s commented Dec 21, 2020

The Linux CIs that may benefit most from this are 10 times cheaper per minute than MAC image

Yes, this is the reason we/I also use Linux builds in most places.

Popular packages such as sf and tmap will work on the cheapest distro out of the box with usethis::use_tic()

I certain cases this might apply yes - if the sysreqs are specified and/or cover all deps of their own R package deps.

Add an argument like use_sysreqs = FALSE to enable the user to choose whether or not the partial solution adding system requirements is used, setting it as FALSE by default would I think mitigate all the negatives you outline above.

An argument seems simple in the first place but might not be so simple with respect to implementation and clarity. In use_tic() all choicse usually apply to all settings and a global argument which only triggers a change for Linux based builds is probably confusing.
Also it would need some conditional template creation or another template just for this purpose.
Given that I would rather favor a well commented default entry into the templates.

Thoughts? Happy to help out with the implementation of the idea if that would help and as I say don't see this PR as ready - just hoping to get a conversation going. Hoping words can lead to positive change (vested interest: I hope to use this functionality if it becomes available)!

Happy to include new features which are requested if they help people in practice. Also thanks for contributing and "fighting" for this feature despite my initial doubts ;)
{tic} hasn't seen much of it yet, maybe due to its complexity.

Would you mind updating your PR with some detailed comments? I might polish and add it to all other templates then (you can also do this if you want of course).

@Robinlovelace
Copy link
Member Author

Would you mind updating your PR with some detailed comments? I might polish and add it to all other templates then (you can also do this if you want of course).

Sure. Thanks for the constructive comments. Will add comments in place. Any idea how to make the checks pass?

@Robinlovelace
Copy link
Member Author

I've updated the PR in any case with comments. If there's a way to 'auto-uncomment' this from the R command line I'm in favour.

@pat-s
Copy link
Member

pat-s commented Dec 24, 2020

I've updated the PR in any case with comments. If there's a way to 'auto-uncomment' this from the R command line I'm in favour.

I'm fine with having it on by default - I just meant to say that an descriptive comment above that block would be great for users to understand the limitations :)

Any idea how to make the checks pass?

There are checks which compare the templates before and after applying update_yml(). These are now failing because you only modified the template but not the new solution.

@Robinlovelace
Copy link
Member Author

Many thanks for persevering with this @pat-s hope to use the results!

@pat-s
Copy link
Member

pat-s commented Jun 25, 2021

Sorry for waiting so long 🙈 too many projects..

@pat-s pat-s merged commit a9d28d0 into ropensci:main Jun 25, 2021
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.

2 participants