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

Avoid warnings with R hooks when renv version don't match #1841

Merged
merged 1 commit into from May 5, 2021

Conversation

lorenzwalthert
Copy link
Contributor

@lorenzwalthert lorenzwalthert commented Mar 16, 2021

Attempts to solve #1835 as initially outlined in #1835 (comment). To further avoid warnings for differing R versions, we suppress warning messages on environment loading. To not write infrastructure like .Rprofile, we use renv::load() over renv::activate(). For installing additional dependencies, we also added skipping of the startup process. I think after #1878, I might have fixed the biggest bugs I created -.-

@lorenzwalthert
Copy link
Contributor Author

@asottile can you have a look at this and tell me if it makes sense, also in the light of our discussion in #1835? I used this locally for a few weeks and it worked fine. Maybe we need to figure out licensing for the {renv} files.

@asottile
Copy link
Member

yeah seems fine! hmm yeah licensing I didn't think about that -- perhaps we can make a ./licenses directory and add license_files = licenses/renv_LICENCE or whatever (assuming it is permissively licensed, if it isn't this might be a dead end)

@lorenzwalthert
Copy link
Contributor Author

lorenzwalthert commented Apr 17, 2021

{renv} is MIT licensed, as pre-commit, so maybe we don’t need to include additional licenses? We can also ask in https://github.com/rstudio/renv how they suggest to handle this, because essentially every project that uses {renv} has this file in version control, and I haven’t seen any special copyright attribution in any project that uses {renv} (although our case might not be 100% the same).

@asottile
Copy link
Member

I would rather lean on the side of caution here -- it's not difficult to abide by the license terms -- we just need a copy of their license in our source tree and our distributables

we could even put it in the resources directory so it gets copied for the empty template into renv just to be on the safe side

sorry for the slow response, things have been crazy recently

@lorenzwalthert
Copy link
Contributor Author

lorenzwalthert commented Apr 20, 2021

sorry for the slow response, things have been crazy recently

No problem.

we just need a copy of their license in our source tree and our distributables

So licensing for R packages work usually like this: Put the type of license in the DESCRIPTION file as a field (only a few ones are permitted). For {renv}, it's MIT + file LICENCE. Then, there is a separate file called LICENSE, which can have the actual licence or just the copyright holders. {renv} has only the latter, so then I guess we should include a MIT Licence and add the copyright and year from LICENSE.

we could even put it in the resources directory so it gets copied for the empty template into renv just to be on the safe side

Agree

@asottile
Copy link
Member

hmmmmm given the contents of that file, I'm not sure they've licensed it for redistribution -- we would probably want to clarify with them what terms that it is allowed to be copied and redistributed.

I don't see an actual license file in their repo beyond that 2-liner (which probably has no legal precedence). I think it would be wrong to take that and stuff it into a MIT license (I think that would be relicensing without approval) -- we should probably start an issue thread on their side unless there's an actual copyable license file!

@lorenzwalthert
Copy link
Contributor Author

Sure, I can open an issue in their repo so we can figure things out and be on the Safe side. I know the authors and I think they don’t have fundamental objections to what we are doing here.

@kevinushey
Copy link

I don't see an actual license file in their repo beyond that 2-liner (which probably has no legal precedence). I think it would be wrong to take that and stuff it into a MIT license (I think that would be relicensing without approval) -- we should probably start an issue thread on their side unless there's an actual copyable license file!

FWIW this is just the canonical way that R packages (at least, those destined for CRAN) declare that they are MIT-licensed (which I agree is a bit strange).

But perhaps I can change this so the, shall we say, CRAN-compatible-declaration-of-MIT-license version of the LICENSE file is used on CRAN, but the "real" version is used on GitHub.

@asottile
Copy link
Member

I don't see an actual license file in their repo beyond that 2-liner (which probably has no legal precedence). I think it would be wrong to take that and stuff it into a MIT license (I think that would be relicensing without approval) -- we should probably start an issue thread on their side unless there's an actual copyable license file!

FWIW this is just the canonical way that R packages (at least, those destined for CRAN) declare that they are MIT-licensed (which I agree is a bit strange).

But perhaps I can change this so the, shall we say, CRAN-compatible-declaration-of-MIT-license version of the LICENSE file is used on CRAN, but the "real" version is used on GitHub.

yeah I think it would be sufficient for my uses to have a LICENSE.CRAN and a LICENSE or whatever naming you're comfortable with (this would also make it so github auto-detects your license too)

@lorenzwalthert
Copy link
Contributor Author

lorenzwalthert commented Apr 27, 2021

But perhaps I can change this so the, shall we say, CRAN-compatible-declaration-of-MIT-license version of the LICENSE file is used on CRAN, but the "real" version is used on GitHub.

I think at some point, LICENSE used to contain the actual license text. If that is still CRAN compatible, would it maybe also be an option?

@kevinushey
Copy link

I think at some point, LICENSE used to contain the actual license text. If that is still CRAN compatible, would it maybe also be an option?

Unfortunately this is explicitly not accepted by CRAN. 😞

@kevinushey
Copy link

I've made a change now so that:

  • The canonical source code at https://github.com/rstudio/renv includes the standard MIT license file;
  • When preparing a tarball for CRAN (via R CMD build), a tarball with the CRAN-compatible declaration of the MIT license is used instead.

Hopefully this is a fair compromise.

@asottile
Copy link
Member

@kevinushey thanks a ton! that's super helpful!

@lorenzwalthert
Copy link
Contributor Author

Great. @asottile how do we progress from here? From the {renv} authors: rstudio/renv#738

These files are (at least currently) also MIT-licensed, and so I think they're fine to use in any project that is licensed in an MIT-compatible way. That is, please feel free to use them with pre-commit.

The intent is definitely to allow users to freely use those files in their projects without needing to worry about copyright / licensing occurs, so we'll evaluate if unlicense might be a good choice.

@asottile
Copy link
Member

for now let's take their LICENSE file and also place that into renv directories as a resource

@lorenzwalthert
Copy link
Contributor Author

Please see c3d186d how I added the licenses:

  • pre_commit/resources/empty_template_LICENSE.renv (will be copied into the users renv/ folder for local hooks, where the activate.R resides)
  • testing/resources/r_hooks_repo/renv/LICENSE (where an activate.R file resides for testing)

Is that what you had in mind?

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants