search

remindmodel / remind

REMIND - REgional Model of INvestments and Development
Other
99 stars 129 forks source link

reset renv/activate.R before sourcing it #1725

Closed pascal-sauer closed 3 months ago

pascal-sauer commented 3 months ago

Purpose of this PR

reset renv/activate.R before sourcing it, preventing the error object '..version..' not found that comes up when jumping from renv version 0.16.0 to 1.0.7 (maybe also vice versa)

Type of change

(Make sure to delete from the Type-of-change list the items not relevant to your PR)

Checklist:

Further information (optional):

dklein-pik commented 3 months ago

What ist the role of RESET_RENV_ACTIVATE_SCRIPT? Where is it potentially set and why?

pascal-sauer commented 3 months ago

It is not set automatically anywhere, but if you do not want your renv/activate.R script to be reset you can manually set RESET_RENV_ACTIVATE_SCRIPT to FALSE.

orichters commented 3 months ago

Sorry, but did you test that? You seem want to load https://github.com/dklein-pik/remind/blob/b83bb1811ff08d8ee5ba8e834af5dd0080d10e66/renv/activate.R but this commit isn't part of the normal repo. So it fails with

error: pathspec 'renv/activate.R' did not match any file(s) known to git

Really, test the changes before you merge these kind of changes.

dklein-pik commented 3 months ago

Sorry, I cant reproduce this error. Neither with a freshly cloned remind nor when updating an older version.

dklein-pik commented 3 months ago

For me this b83bb1811ff08d8ee5ba8e834af5dd0080d10e66 points to https://github.com/remindmodel/remind/commit/b83bb1811ff08d8ee5ba8e834af5dd0080d10e66

orichters commented 3 months ago

Ok, thanks for checking! grep activate /p/tmp/oliverr/NGFS_v5/d_remind-3p3p1-hpc/output/C_h_cpol_KLW_d50_newmagicc-rem-1/log.txt is cluttered with these messages. Don't know exactly what is going on there, but it also does not seem to really bother the run…

I check again with the latest develop.

orichters commented 3 months ago

Sorry, I first made an error, when I claimed that current develop was fine. Also grep activate /p/tmp/oliverr/remind-renvtest/output/testOneRegi/log.txt shows this error message. But I have to admit that testOneRegi is not failing, it seems to basically ignore it.

orichters commented 3 months ago

Seems the problem is: if you start R or Rscript from the outputfolder such as output/testOneRegi, then it cannot find the file, as it lies at ../../renv/activate.R according to git.

orichters commented 3 months ago

Maybe:

git show b83bb1811ff08d8ee5ba8e834af5dd0080d10e66^:renv/activate.R > renv/activate.R
pascal-sauer commented 3 months ago

1730 should fix this

pascal-sauer commented 3 months ago

Sorry, but did you test that? You seem want to load https://github.com/dklein-pik/remind/blob/b83bb1811ff08d8ee5ba8e834af5dd0080d10e66/renv/activate.R but this commit isn't part of the normal repo. So it fails with

error: pathspec 'renv/activate.R' did not match any file(s) known to git

Really, test the changes before you merge these kind of changes.

I feel offended and pressured by this message. To me it reads like I should feel like a careless idiot which is not acceptable. Needless to say being afraid of this kind of messages makes it much less likely that I'll contribute to remind in the future.

pascal-sauer commented 3 months ago

With that out of the way, you are right, I did not properly test this. Then again, it's not like this broke remind or anything, it produced a couple junk lines in a log script without stopping or breaking anything.

pascal-sauer commented 3 months ago

Your git show approach is cool, I didn't know about that before, thanks! In my PR I went with an additional check instead, since the runfolder renv setup should not be changed at all, but it ultimately shouldn't matter