https://pantsbuild.org/ logo
#development
Title
# development
h

hundreds-breakfast-49010

09/05/2019, 10:23 PM
how should I generate a
UrlToFetch
from a
BinaryToolBase
?
a

aloof-angle-91616

09/05/2019, 10:23 PM
use the
.get_external_url_generator()
method to get a url generator!
and use the default one in
binary_util.py
if that returns None
the one named
class PantsHosted(BinaryToolUrlGenerator):
w

witty-crayon-22786

09/05/2019, 11:14 PM
there was a previous thread on this. the challenge is that you need a digest per platform
(see rest of thread)
a

aloof-angle-91616

09/05/2019, 11:16 PM
(1) isn't that concept what @early-needle-54791 is enabling with multiplatform process requests? (2) cloc is not platform dependent
also, i described to @hundreds-breakfast-49010 how to do this already with platform awareness
I'm trying ot figure out how the
PantsHosted
class you mentioned works [3:28 PM] cosmicexplorer [3:28 PM] you take the
binary_request
(which
BinaryToolBase
generates) and add the
baseurls
which are a bootstrap option (
--binaries-baseurls
) [3:28 PM] (see the
__init__()
) [3:28 PM] then it just works [3:29 PM] Greg Shuflin [3:29 PM] should
_make_binary_request
be public? [3:30 PM] cosmicexplorer [3:30 PM] it could be made public [3:30 PM] it's still an implementation detail imho [3:30 PM] well [3:30 PM] no [3:30 PM] Greg Shuflin [3:30 PM] maybe
ClocBinary
could have a method that returns a
UrlToFetch
? [3:30 PM] cosmicexplorer [3:30 PM] what i wanted to do was to actually make a rule from
BinaryRequest -> Digest
[3:31 PM] Greg Shuflin [3:31 PM] where should that rule live? [3:31 PM] cosmicexplorer [3:31 PM] i wouldn't put it on
ClocBinary
[3:31 PM]
binary_tool.py
[3:31 PM] we have all the info we need already in
BinaryToolBase
and
PantsHosted
(edited) [3:31 PM] Greg Shuflin [3:31 PM]
PantsHosted
relies on having a baseurl [3:31 PM] cosmicexplorer [3:31 PM] yes [3:32 PM] Greg Shuflin [3:32 PM] which
BinaryUtil
provides [3:32 PM] cosmicexplorer [3:32 PM] that's a bootstrap option [3:32 PM]
--binaries-baseurls
[3:32 PM] yes [3:32 PM] you can make an
optionable_rule()
[3:32 PM] if we can make
_make_binary_request
public, then make a ruleset from
BinaryRequest -> Digest
which tries all the urls provided for a given
BinaryRequest
via invoking the url generator, i think that would work [3:33 PM] Greg Shuflin [3:33 PM] and that ruleset would also rely on
BinaryUtil
as an input [3:33 PM]
@rule(Digest [BinaryRequest, BinaryUtil])
? [3:36 PM] cosmicexplorer [3:36 PM] yes [3:36 PM] i think [3:36 PM] and then declaring an
optionable_rule(BinaryUtil)
so that it can be consumed [3:38 PM] and with the rule you just described (
@rule(Digest, [BinaryRequest, BinaryUtil]
), anyone who wants a binary tool can just say:``` @rule(Whatever, [ClocBinary]) def do_cloc(cloc_binary): cloc_snapshot = yield Get(Digest, BinaryRequest, cloc_binary.make_binary_request()) # do whatever with it def rules(): return [ do_cloc, optionable_rule(ClocBinary), ] ```
w

witty-crayon-22786

09/05/2019, 11:18 PM
1)
only for ExecuteProcessRequest... would need something similar for binaries.
2)
mm, yep. just one digest then.
a

aloof-angle-91616

09/05/2019, 11:18 PM
so in short, by making a rule from
(BinaryRequest, BinaryUtil) -> Digest
we can avoid marrying a
Subsystem
in general with a
UrlToFetch
and we can continue to use the same classes we've been using, just using a different method than
.select()
this time
and
BinaryRequest
contains the platform
is why this is feasible
hm no
w

witty-crayon-22786

09/05/2019, 11:19 PM
k. still need to add a tree of digests to whatever subsystem.
a

aloof-angle-91616

09/05/2019, 11:20 PM
still need to add a tree of digests
what does this mean? making
.make_binary_request()
public in
BinaryToolbase
(allowing the
@rule
at the end of my above quoted conversation) gives us all the information we need to make a platform-specific binary request, without attaching any new information to any subsystem
h

hundreds-breakfast-49010

09/05/2019, 11:22 PM
I'm confused too, and also about what this rule would have to do with
ExecuteProcessRequest
once we have a rule that yields a
Digest
we can do whatever we want with it, whether it's use it in
ExecuteProcessRequest
or something else, right?
a

aloof-angle-91616

09/05/2019, 11:22 PM
yes!
i think
ExecuteProcessRequest
is not relevant now, i brought it up because i wasn't sure why the platform-specificity was seen as a blocker
h

hundreds-breakfast-49010

09/05/2019, 11:23 PM
w

witty-crayon-22786

09/05/2019, 11:23 PM
the reason
--version
is an option for these tools is that people want to change the version
a

aloof-angle-91616

09/05/2019, 11:23 PM
yes
w

witty-crayon-22786

09/05/2019, 11:23 PM
when they do that, they will also need to change the digests
a

aloof-angle-91616

09/05/2019, 11:23 PM
yes
w

witty-crayon-22786

09/05/2019, 11:23 PM
and they will need a digest per platform
a

aloof-angle-91616

09/05/2019, 11:24 PM
this is all in the
BinaryRequest
object
w

witty-crayon-22786

09/05/2019, 11:24 PM
maybe that's a
dict_option
rather than a tree, but.
a

aloof-angle-91616

09/05/2019, 11:24 PM
that's only if they need to download it for both platforms
i don't think we want to do that automatically
h

hundreds-breakfast-49010

09/05/2019, 11:24 PM
so passing a new`--version` flag will make the
BinaryRequest
object be different (and invalidate any caching)?
👍 1
w

witty-crayon-22786

09/05/2019, 11:24 PM
you'd... probably have to? 99% of usecases involve both linux and osx.
a

aloof-angle-91616

09/05/2019, 11:24 PM
like i wouldn't want to remote the use of cloc i would think?
w

witty-crayon-22786

09/05/2019, 11:24 PM
we already do
a

aloof-angle-91616

09/05/2019, 11:25 PM
ok
i don't know if i agree with the "99% of usecases" part but returning a dict is still not hard to do and can be done at the python level
w

witty-crayon-22786

09/05/2019, 11:26 PM
not disagreeing. that's just why i was mentioning a subsystem / place for the new option(s).
a

aloof-angle-91616

09/05/2019, 11:26 PM
where would the new options come from?
a

aloof-angle-91616

09/05/2019, 11:27 PM
i'm really having difficulty understanding your point
does changing options not invalidate
optionable_rule()
instances?
w

witty-crayon-22786

09/05/2019, 11:28 PM
if i set
./pants --cloc-binary-version=0.10.0-mycustom-version cloc
, it will break, because any hardcoded digest will mismatch
i must also specify a new digest
as an option
💡 1
a

aloof-angle-91616

09/05/2019, 11:28 PM
oh incredible
@hundreds-breakfast-49010 ^
thank you very much for explaining stu!
i had forgotten that we provide digests for
UrlToFetch
it might still be something we can stick onto
BinaryToolBase
but yeah
h

hundreds-breakfast-49010

09/05/2019, 11:30 PM
I'm confused about the hardcoded Digest
a

aloof-angle-91616

09/05/2019, 11:30 PM
when we do a
UrlToFetch
, we also provide an expected digest
if it doesn't match the result, it fails
h

hundreds-breakfast-49010

09/05/2019, 11:30 PM
hm, ok
a

aloof-angle-91616

09/05/2019, 11:31 PM
so while we can hardcode in some digests for some known versions, making it an option is necessary
h

hundreds-breakfast-49010

09/05/2019, 11:31 PM
by "making it an option" do you mean inviting the user to pass in the digest hex string as a command-line argument?
a

aloof-angle-91616

09/05/2019, 11:31 PM
or an env var, or a config file entry
so yes
would recommend thinking about the user interface for this a bit
h

hundreds-breakfast-49010

09/05/2019, 11:32 PM
hm, ok
a

aloof-angle-91616

09/05/2019, 11:32 PM
if there's a way to split it off from implementing the basic functionality you're looking for with cloc i might recommend it, but it would make sense to have it together
h

hundreds-breakfast-49010

09/05/2019, 11:32 PM
well the whole reason I'm going down this rabbit hole
a

aloof-angle-91616

09/05/2019, 11:32 PM
right
h

hundreds-breakfast-49010

09/05/2019, 11:32 PM
is because i currently get the cloc binary from calling
.select()
on an instance of
ClocBinary
and this appraently won't work when remoting
a

aloof-angle-91616

09/05/2019, 11:32 PM
yes
h

hundreds-breakfast-49010

09/05/2019, 11:33 PM
because it points at a local file system location
a

aloof-angle-91616

09/05/2019, 11:33 PM
only `ExecuteProcessRequest`s work remotably
yes
so
especially since
cloc
isn't platform-dependent
h

hundreds-breakfast-49010

09/05/2019, 11:33 PM
so right now something is successfully downloading the cloc perl script to that location (maybe it's not checking a hash?)
a

aloof-angle-91616

09/05/2019, 11:33 PM
you could do a hardcoded digest for now
h

hundreds-breakfast-49010

09/05/2019, 11:33 PM
but whatever it is is all v1 code that I don't understand
a

aloof-angle-91616

09/05/2019, 11:33 PM
it's all in
binary_util.py
h

hundreds-breakfast-49010

09/05/2019, 11:34 PM
so I just want to do whatever the right thing to do is, in a v2 context
a

aloof-angle-91616

09/05/2019, 11:34 PM
it's complex and annoying code iirc
yes
h

hundreds-breakfast-49010

09/05/2019, 11:34 PM
so that's the
select()
method tehre
a

aloof-angle-91616

09/05/2019, 11:34 PM
so a
BinaryRequest -> Digest
rule would be the right thing
h

hundreds-breakfast-49010

09/05/2019, 11:34 PM
ok
a

aloof-angle-91616

09/05/2019, 11:34 PM
(imho)
h

hundreds-breakfast-49010

09/05/2019, 11:35 PM
and internally to that rule, I'll need to invoke
UrlToDownload -> Snapshot
a

aloof-angle-91616

09/05/2019, 11:35 PM
yes
h

hundreds-breakfast-49010

09/05/2019, 11:35 PM
which I think we're already using
a

aloof-angle-91616

09/05/2019, 11:35 PM
yes
h

hundreds-breakfast-49010

09/05/2019, 11:35 PM
and that's the thing that expects a precomputed digest
👍 1
a

aloof-angle-91616

09/05/2019, 11:35 PM
that's correct!
h

hundreds-breakfast-49010

09/05/2019, 11:35 PM
and here digest is just, like, a sha-256 hash of the file or something?
not entirely sure what that is
a

aloof-angle-91616

09/05/2019, 11:35 PM
yes
well
no
i believe it's a hash of the protobuf representing that directory containing the file
i can check
h

hundreds-breakfast-49010

09/05/2019, 11:36 PM
I guess I can just run it with a bogus string and hope the error message reports the correct hash, which I can then hardcode
🔥 1
a

aloof-angle-91616

09/05/2019, 11:37 PM
absolutely 100% correct
h

hundreds-breakfast-49010

09/05/2019, 11:37 PM
I think we're also assuming a specific cloc version
I didn't actually know there was a command line flag for specifying different versions of the cloc binary
a

aloof-angle-91616

09/05/2019, 11:37 PM
you can bolster this investigation by using ripgrep or something to search for
UrlToFetch
(or
UrlToDownload
?) and see whether the error message reports the correct hash
h

hundreds-breakfast-49010

09/05/2019, 11:37 PM
maybe we should just remove that? does anyone really care about being able to specify this?
a

aloof-angle-91616

09/05/2019, 11:37 PM
yes
because it's an option on all
BinaryToolBase
subclasses too
and many of those definitely care about their version
what we can do is provide a default value for the
version -> digest
dict_option
(stu was mentioning this earlier)
and then if users have their own version, they can simply edit the option
well
wait
sorry
no wait yes it's a
dict_option
because of multiple platforms!
so like
Copy code
{'linux': ('1.0.0', 'asdfasdfsadf'),
 'darwin': ('1.0.0', 'asdfasdfdf')}
h

hundreds-breakfast-49010

09/05/2019, 11:42 PM
I'm not sure where this
dict_option
you're talking about would fit in
a

aloof-angle-91616

09/05/2019, 11:44 PM
we would convert the
--version
option which is registered on
BinaryToolBase
into an option named something like
--version-digests
which would have the value like the dict i messaged just above
hm or
no that's right
h

hundreds-breakfast-49010

09/05/2019, 11:50 PM
so that would entail changing the
version
method on
BinaryToolBase
?
ah that's only being used on`select`
a

aloof-angle-91616

09/05/2019, 11:50 PM
i think it would entail changing the
register_options()
method, actually
h

hundreds-breakfast-49010

09/05/2019, 11:50 PM
at least in that file
a

aloof-angle-91616

09/05/2019, 11:51 PM
it's also being used to make a binary request
h

hundreds-breakfast-49010

09/05/2019, 11:51 PM
yeah that's what
select
is doing with it
a

aloof-angle-91616

09/05/2019, 11:51 PM
yes
h

hundreds-breakfast-49010

09/05/2019, 11:54 PM
can we just change option names like that?
a

aloof-angle-91616

09/05/2019, 11:54 PM
i was thinking of adding a new option and deprecating the old one
options are really easy to deprecate
h

hundreds-breakfast-49010

09/05/2019, 11:59 PM
do we have an example of that?
a

aloof-angle-91616

09/05/2019, 11:59 PM
i always just search the repo with ripgrep whenever i want an example
src/python/pants/option/optionable.py
might have the description of the correct keyword arguments
looking at options registered in
register_options()
with the
removal_version
kwarg should be what you want to do