Closed seth127 closed 1 year ago
I think I recall getting myself turned around about this at some point.
So, solution 3 is essentially just making the error message clearer, right?
I'd vote against your lowest-ranked 4 for compatibility reasons.
I'm a bit confused about solution 1. Given that users may be setting this on-disk in .Rprofile or whatnot, isn't the question going to just keep popping up? If so, this feels like it would add more noise than it's worth, and I'd prefer solution 2 or 3. I don't spot any issue with 3 (using directory silently), though I don't love the idea of making bbr.bbi_exe_path
handling conditional/branching just because of the behavior of use_bbi
.
Here's another proposal: make use_bbi()
error if the default value returned by build_bbi_install_path()
is a directory. That 'd catch thebbr.bbi_exe_path
-derived default, and, as far as I can tell, that function should never return a directory. So, that's basically number 4, but avoids breaking the case where a directory is explicitly passed as .path
.
Given that users may be setting this on-disk in .Rprofile or whatnot, isn't the question going to just keep popping up?
Hmmm, this is a good point. That probably doesn't make much sense then.
solution 3 is essentially just making the error message clearer, right?
Yea pretty much.
I don't spot any issue with 3 (using directory silently), though I don't love the idea of making bbr.bbi_exe_path handling conditional/branching just because of the behavior of use_bbi.
(note: I think you mean "any issue with 2") This feels like the most user-friendly option at this point, but I also don't love that convoluted conditional handling.
I like your proposal as well, and I think it's definitely the better version of option 4.
We'll have to think on this a bit more. Thanks for the input.
(note: I think you mean "any issue with 2")
Yep, sorry for the mixup.
@kyleam ok, upon further thought, I'm leaning towards option 2 (from above), more specifically: let bbi_exec()
optionally take a directory path and look for bbi
inside that directory.
My thinking is that, as stated above, use_bbi()
essentially already does this:
use_bbi()
can accept either:
- a path to a file (the path where the installed executable will be)
- a path to a directory that the executable should be installed into. If it gets a directory path (i.e.
"/data/apps"
) it will install the binary within that directory (i.e."/data/apps/bbi"
).
We would essentially be making things more consistent by allowing bbi_exec()
to behave the same way.
options("bbr.bbi_exe_path()")
can now be set to a directory containing bbi
. Will this cause any problems? I can't think of any...Quickly glancing over this, I think I still agree with everything I wrote, so I'm okay with what you propose but have a slight preference for the error.
but I don't think it will really be complicated or opaque at all. Am I missing something here?
I don't think the logic is particularly complicated (otherwise it would be an objection rather than a slight preference for another option). That doesn't mean that it doesn't add complexity in terms of what users/developers need to think about and test. It all adds up, and things that don't seem bad in isolation lead to execution that is hard to reason about.
My current opinion is that we should error when .path
resolves to a directory (my option 4, at top). Partially based on @kyleam 's reasoning above, but also on the details below.
I started looking into this a bit more and I'm reassessing this previous statement:
use_bbi() can accept... a path to a directory that the executable should be installed into.
While it is true that it can accept a path to a directory and that it will install into this directory, I'm beginning to consider this more of a bug than a feature. My reasoning:
→ Please either set options...
message (highlighted in the second example below) is incorrect and leads the user to a non-working state.file.copy()
(more detail below).dir
, but was changed here. My feeling is that the legacy behavior persists, perhaps accidentally, but certainly confusingly. (Somewhat embarrassingly, we still mention .dir
in the docs, so we definitely need to fix that.)We confirm that having options("bbr.bbi_exe_path")
set to a dir causes use_bbi()
to install to {options("bbr.bbi_exe_path")}/bbi
, which is then not found.
We confirm that this doesn't work correctly either. That is, when you pass a directory to use_bbi(.path)
, it actually installs bbi
to {.path}/bbi
and then erroneously tells you that it has has installed it to .path
instead.
It even goes so far as to encourage you to → Please either set options('bbr.bbi_exe_path' = '{.path}') in your .Rprofile...
, though doing this does not actually work.
The reason this happens comes down to the fact that we use file.copy(), which, when it receives a directory for its second argument, will copy the file into that directory.
While it is true that it can accept a path to a directory and that it will install into this directory, I'm beginning to consider this more of a bug than a feature. My reasoning:
Thanks for the analysis. I find that convincing and think that removes my backward compatibility concern about option 4. Sounds like a good direction to go to me.
New requirement, added to story CFG-S004
:
UBI-R006:
description: use_bbi should take a path to the intended location of the executable file
tests:
Current state
options("bbr.bbi_exe_path")
needs to be set to a path to a workingbbi
executable (i.e. the file)use_bbi()
defaults to installingbbi
to the path set inoptions("bbr.bbi_exe_path")
.use_bbi()
can accept either:"/data/apps"
) it will install the binary within that directory (i.e."/data/apps/bbi"
).options("bbr.bbi_exe_path")
is set to a directory,check_bbi_exe()
will fail whenever something tries to callbbi
(i.e. it won't be able to find the executable)Resulting confusion
This results in a confusing state where the user can do the following:
options("bbr.bbi_exe_path" = "/data/apps")
use_bbi()
and havebbi
successfully install to/data/apps/bbi
bbi
and have it fail, saying it can't findbbi
at/data/apps
(This actually happened in a MeRGE training, which is where this issue came from.)
Solution options
Below are some options I see. They are in order of my current preference, though I'm very open to feedback.
bbi_exec()
(or likelycheck_bbi_exe()
) is passed a directory path, let it look forbbi
inside that directory, and (if found) ask the user if they would like to resetoptions("bbr.bbi_exe_path")
to the executable path. a. Potentially, first check thatoptions("bbr.bbi_exe_path")
is indeed set to that directory path, because at that point we won't technically know if that's where the path came from. b. If it isn't set to that directory path (again, I think this is unlikely, but possible) we proceed to error, as we currently do here.bbi_exec()
(or likelybbi_check_exe()
) is passed a directory path, let it silently look forbbi
inside that directory, and use that instead.bbi_exec()
(or likelybbi_check_exe()
) is passed a directory path, error and say something like "some/dir/you/passed
is a directory, did you meansome/dir/you/passed/bbi
?"use_bbi()
no longer accept a directory path, so that we catch the issue sooner and make sure users are setting the path to the executable instead of the directory.