https://pantsbuild.org/ logo
Join Slack
Powered by
<@UKPS08ZGW>: hey! so re: your question about `Typ...
# development
w
@hundreds-breakfast-49010: hey! so re: your question about `TypeId`s and https://github.com/pantsbuild/pants/issues/7819
depending how many situations you need to do the lookup in, you might consider adding an extern function that does something like: 
fn is_union(TypeId) -> Option<TypeId>
this might be complicated by the fact that we essentially "erase" the type of the union here: https://github.com/pantsbuild/pants/blob/62b566fb55096bc8ef44864667813257e792cb62/src/python/pants/engine/rules.py#L440-L448
h
interesting, so basically leave the mapping of a type to it's union on the python side
w
um, maybe? yea. but you could also pass that across to rust
oh, so. here is one wrench: a type might be a member of multiple unions
oh oh.
so: a 
@union
can only possibly by used with the three argument form of 
Get
. example: https://github.com/pantsbuild/pants/blob/62b566fb55096bc8ef44864667813257e792cb62/src/python/pants/rules/core/test.py#L74-L76
what that means is that if this code passes those three arguments across unadulterated: https://github.com/pantsbuild/pants/blob/62b566fb55096bc8ef44864667813257e792cb62/src/python/pants/engine/native.py#L429-L444 ... then we would have two types: the type we were expecting, and the type we were given
that would help enrich this error, because when we fail, we could call back with something like 
fn is_union(TypeId) -> Option<TypeId>
to see whether we failed due to a union, or just a type mismatch
h
does the 
PyGeneratorResponse::Get
variant directly correspond to the 3-argument Get call in python?
looks like that's ultimately being used on the rust side in 
generator_send
in externs.rs
w
yep
the two and three are forms are both converted into the three arg form under the hood
h
I think I've gotten a bit lost in the code here
w
@hundreds-breakfast-49010: i would say: try reproing the error, and then adding print statements
h
the ultimate problem to be solved is making the error message in https://github.com/pantsbuild/pants/blob/f7d45dcc0e7ffa3f84d5fc99655a68e4d8ecd564/src/rust/engine/src/nodes.rs#L127 better, right?
w
yes
h
ah, yeah
so what's the best way to repro the error?
I still haven't acutaly used pants very much
and I'm not sure how to set up a working example on myown system that will exercise this code path
w
./pants --no-v1 --v2 test $something-that-is-not-a-python-test
h
those flags are saying "only use the v2 engine" I assume
can that variable be literally any random string?
oh no it probably has to be a real target
w
no
yea
h
so a target in pants can be any file right?
w
so this should work:
Copy code
./pants --no-v1 --v2 test 3rdparty/jvm/com/fasterxml/jackson/module:scala
sorry, editted
@hundreds-breakfast-49010: no: targets in pants are declared in BUILD files
h
ah right
w
here's a better one:
h
okay so in that BUILD file there's the 
jar_library
function and that has a name and that's where 'scala' comes from
w
Copy code
./pants --no-v1 --v2 test examples/src/java/org/pantsbuild/example/hello/simple
yep
this target is a 
jvm_binary
, which is definitely not a member of the test target union
so you get noise like
Copy code
Exception: WithDeps(Inner(InnerEntry { params: {Address, OptionsBootstrapper}, rule: Task(Task { product: TestResult, clause: [Select { product: HydratedTarget }], gets: [Get { product: TestResult, subject: PythonTestsAdaptor }], func: coordinator_of_tests(), cacheable: true }) })) did not declare a dependency on JustGet(Get { product: TestResult, subject: JvmBinaryAdaptor })
and in particular, the type that we're trying to use is 
JvmBinaryAdaptor
h
I don't see JvmBinaryAdaptor in my own output
Copy code
Exception: WithDeps(Inner(InnerEntry { params: {Address, OptionsBootstrapper}, rule: Task(Task { product: TestResult, clause: [Select { product: HydratedTarget }], gets: [Get { product: TestResult, subject
: PythonTestsAdaptor }], func: coordinator_of_tests(), cacheable: true }) })) did not declare a dependency on JustGet(Get { product: TestResult, subject: TargetAdaptor })
w
See the second command I gave
Copy code
./pants --no-v1 --v2 test examples/src/java/org/pantsbuild/example/hello/simple
It's a better example because the type error is more obvious
h
so, 
get_type_for
expects an argument of type 
Value
, whihc seems to be a wrapper around a c void pointer
I'm thinking, maybe I want a similar extern fn 
get_union_for
👍 1
and in the ok_or_else block in nodes.rs where the failure happens, I'd want to call that on 
dependency_key
, and that would give me back an Option<TypeId>
if it's Some(x), we have a TypeId corresponding to the Union and we can make the error message do something ueful with it
if it's none, then we just print what we're currently printing
does that make sense?
w
Yea, sounds good!
You should see the links in the concrete proposal I posted too though: it's possible that you will need to change the rust signature for the generator response 
Get
h
right that's PyGeneratorResponse::Get
I'm not sure where that variant shows up in 
src/nodes.rs
though
dependency_key
is a 
Get
but that's not the same 
Get
as that variant of 
PyGeneratorResponse
, right?
and if I'm making a new function to call back into the python get the union information for 
dependency_key
, I'm not sure why that would entail changing how 
PyGeneratorResponse::Get
works
but I'm also not sure how to write the union lookup function on the python side or why that needs to be in the python
sorry if these are basic questions I think my mental model of what exactly is going on with these data structures is still incomplete
w
Try it out, and when you don't see your new error message triggering, you can take another look at my comment
h
so I think I've got this mostly figured out
except that I'm not sure how to write the FFI function that takes a 
TypeId
on the rust side, and gets back the python object corresponding to the class so I can check for the '__is_union' attribute on it
I'm also not sure how to use the 
_extern_decl
decorator to say that the return type of that function is a rust-level 
Option<TypeId>
I can't tell if the return_type there expects python paramaterized type synax, rust synax, or just doesn't support paramaterized types
w
the 
decl
is a c-level decl, not rust
because it's used with 
cffi
, which is not rust aware
i'm not sure (@aloof-angle-91616 or @average-vr-56795 might know) whether 
cbindgen
runs before that code, and whether the types it generates would be usable in that position
a
@hundreds-breakfast-49010 these questions are all good questions, the 
@_extern_decl
thing is not supposed to be anything other than better than before
is there a diff with your current code i can see?
can look into your question async
w
h
I'm chatting with john about the design of this right now
but basically I think I want a FFI function that returns an 
Option<TypeId>
into rust
a
i think 
rust-bindgen
(or just 
bindgen
?) is the crate that generates rust methods from a C header -- i think 
cbindgen
is the inverse
so need to verify this but @witty-crayon-22786 i think it might be 
cbindgen
running first, then the 
@_extern_decl
code is built including the header file from 
cbindgen
w
ok
hm. so maybe worth experimenting with there. @hundreds-breakfast-49010: you could spend a few minutes adding an Option-like 
enum
on the rust side, and then seeing whether you can use its 
cbindgen
type in that position
the thing that would help with that would be to visually inspect what gets generated by cbindgen (by grepping in the generated code for a keyword like `Broke`: https://github.com/pantsbuild/pants/blob/master/src/python/pants/engine/native.py#L443)
☝️ 1
a
visually inspecting is how i got 
@_extern_decl
working in the first place
h
yeah I'll give that a shot, it would be handy to be able to use Option in FFIs, and there's at least one place where we're constructing a rust enum type in pythonanyway
a
yes, i really agree
h
I think what's important here is that Option is paramaterized in rust though
an Option-like enum should work the same way as 
PyGeneratorResponse
, right?
but 
PyGeneratorResponse
isn't paramaterized
a
i don't think we'd be able to (easily) have something return a literal 
Option<T>
for a specific 
Option<u8>
or something that should have a specific set of C code it can correspond to i think
w
i don't think we'd be able to (easily) have something return a literal 
Option<T>
the confusing thing though is that 
cbindgen
does such a good job with "`enum`s in general"... why would Option not be accounted for?
a
because C doesn't have templates
i would think e.g. an instance 
Option<u8>
for a given 
T
would be very within scope
w
sure. and so for enums in general, it generates a tagged union
a
yes
w
and Option is just an enum
a
i recall seeing a post about it it's very good
w
well, we're using the generated union in the example i linked
(PyGeneratorResponse)
a
The reason it doesn’t handle 
Option
is that 
Option
doesn’t have a 
repr
, so doesn’t have a stable representation
a
yes, @hundreds-breakfast-49010 was asking about the 
PyGeneratorResponse
not being parameterized, and i was saying that in the context of thinking it might be a blocker to making 
PyGeneratorResponse
parameterizable
a
And in fact rust does null-pointer optimising of non-nullable options for space packing
h
Option is just an enum, but it's a parameterized enum
👍 1
w
got it.
a
i didn't make that part clear, thanks
w
because we can't mark it 
repr(c)
, basically
ok. so then creating your own enum and marking it 
repr(c)
would work
h
rust does the null-pointer optimizing thing with Option<&T> because &T is a non-nullable pointer
w
thanks, got it now.
a
Yeah, I think so - I think cbindgen handles parameterised ones fine, as long as they have a repr
h
but in general it can't assume for any Option<T> that the 0 representation of T is illegal
w
@hundreds-breakfast-49010: the 
repr(c)
bit is the overriding factor.
because when you mark it 
repr(c)
you disable a lot of that.
a
rust's type system can make handling edge cases within the system not the worst thing ever
(you're all referring to the rust 
#[repr(C)]
, right??)
@hundreds-breakfast-49010 please let me/us know if the above didn't make it clear enough how to muck about with the 
@_extern_decl
magic, can ping this slack for any qs
w
yea, that
h
sorry was just eating lunch
so, I might need to make my own type that is structurally the same as an Option<T>
that's probably the simplest thing to do right now to get this feature in
I assume 
enum OptionalType { None, Some(TypeId) }
can have a 
#[repr(C)]
?
w
Yep, I think so.
Could be 
ExternOption
or something
h
https://github.com/gshuflin/pants/commit/9c85bd7ebf7907f421017a57e1d7bb34d2e79b51 <- this is blowing up at the cffi stage and I'm not sure why
Copy code
--- stderr
Traceback (most recent call last):
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/cffi/cparser.py", line 276, in _parse
    ast = _get_parser().parse(fullcsource)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/pycparser/c_parser.py", line 152, in parse
    debug=debuglevel)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/pycparser/ply/yacc.py", line 331, in parse
    return self.parseopt_notrack(input, lexer, debug, tracking, tokenfunc)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/pycparser/ply/yacc.py", line 1199, in parseopt_notrack
    tok = call_errorfunc(self.errorfunc, errtoken, self)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/pycparser/ply/yacc.py", line 193, in call_errorfunc
    r = errorfunc(token)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/pycparser/c_parser.py", line 1848, in p_error
    column=self.clex.find_tok_column(p)))
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/pycparser/plyparser.py", line 67, in _parse_error
    raise ParseError("%s: %s" % (coord, msg))
pycparser.plyparser.ParseError: <cdef source string>:3:18: before: extern_get_union_for

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
  File "/home/gregs/code/pants/src/python/pants/engine/native.py", line 148, in bootstrap_c_source
    ffibuilder.cdef(_FFISpecification.format_cffi_externs())
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/cffi/api.py", line 107, in cdef
    self._cdef(csource, override=override, packed=packed)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/cffi/api.py", line 121, in _cdef
    self._parser.parse(csource, override=override, **options)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/cffi/cparser.py", line 315, in parse
    self._internal_parse(csource)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/cffi/cparser.py", line 320, in _internal_parse
    ast, macros, csource = self._parse(csource)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/cffi/cparser.py", line 278, in _parse
    self.convert_pycparser_error(e, csource)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/cffi/cparser.py", line 307, in convert_pycparser_error
    raise CDefError(msg)
cffi.error.CDefError: cannot parse "OptionalTypeId      extern_get_union_for(ExternContext*, Handle*);"
<cdef source string>:3:18: before: extern_get_union_for
Execution of "/home/gregs/code/pants/build-support/bin/native/bootstrap_cffi.sh" failed with exit code Some(1)
you implied I think there was a way to see what the generated output of CFFI on the rust code with respect to that new 
OptionalTypeId
was?
a
check the output of 
ls src/rust/engine/src/cffi/
the 
scheduler.h
is i think generated from cbindgen and the 
native_engine.c
is something we generate from 
@_extern_decl
i believe
h
hm maybe i need to explicitly build the rust or it won't pick up on the new type?
a
"explicitly build the rust" meaning?
h
hm, no, 
./build-support/bin/native/cargo build --manifest-path src/rust/engine/process_execution/Cargo.toml
doesn't seem to add the string 
OptionalTypeId
to any of those files
a
where is 
OptionalTypeId
coming from?
h
that's the new type I defined as #[repr(C)]
in src/engine/src/externs.rs
as far as I can tell I'm doing it hte same way as 
PyGeneratorResponse
a
i would run 
./pants -V
in case it's doing some invalidation that the raw cargo build command isn't
and then recheck the output
h
what's the -V flag for?
a
or deleting the file
it makes pants say the version
same as 
./pants --version
a bit confusing
h
deleting the file and regenerating doesn't make 
OptionalTypeId
show up there
neither does running ./pants -V
a
can you try making it a struct, or can you make the 
NoTypeId
case contain a value? it's possible the cbindgen settings require a flag for that to be on. are you able to edit any of the other structs in the file and see the change in output?
hm
i was going to say it's possible you need to define a 
#[no_mangle] pub extern fn
to go along with it to be visible
not sure
h
hm, editing hte names of existing types (PyGeneratorResponse) and running ./pants --version doesn't seem to generate a new file
a
there's a set of things you need to clear
i run
(i am aware this is absurd)
git clean -xfd src/ && rm -rf ~/.cache/pants/bin/native-engine
which will take a while to recompile rust unless you set 
MODE=debug
because the current rust bootstrap process in the pants repo copies over the native engine binary multiple places we have to clean out more than we might want
h
kk trying that
yeah that regenerates only 
scheduler.h
which still lacks my new type
a
is there any entry for the function you added?
h
no there's not
that's the 
GetUnionForExtern
I added
there is an entry for 
GetTypeForExtern
which is definedin basically the same way right next to it in the file
in 
<http://externs.rs|externs.rs>
I mean
a
showing a diff of 
scheduler.h
before and after your changes might be helpful
i'm p sure i know
i think it's symbols exposed in 
<http://lib.rs|lib.rs>
(so just add your types to that 
use
at the top of 
src/rust/engine/src/lib.rs
?)
oh hm
oh no i think that might be it
because i think the rest are exposed via being arguments to the
Copy code
#[no_mangle]
pub extern "C" fn scheduler_create(
h
yeah the diffs between the two versoins of scheduler.h are null
oh 
externs_set
hm
a
don't have it in front of me right now but https://github.com/eqrion/cbindgen/blob/master/docs.md#writing-your-c-api explicitly says types are exposed transitively via extern fns
h
I think that might've been it
yeah that generated the types in scheduler.h
🎉 1
🔥 1
a
is this enough to unblock you?
h
yup, thanks 🙂
a
ok, great! let me know if we can answer further questions. nobody should be pretending this is at all obvious, this is all stuff we made up
h
it takes time to get familiar with a new codebase
I already feel like Iknow more than I did on friday 🙂
👌 1
a
i also said that in order to impress upon you how a lot of what we do is incredibly groundbreaking
everyone else too
but pants is cool
a
there’s a set of things you need to clear
You shouldn’t need to do this, ever. Any time you do, please find out why and fix it. @aloof-angle-91616
(Or ask for help in doing so)
h
https://github.com/gshuflin/pants/commit/e08dac59d7b4923ca8591ea1f4e1cd0c8b58cc09 <- with this code, I'm running into the following FFI error
Copy code
Fatal Python error: ffi.from_handle() detected that the address passed points to garbage. If it is really the result of ffi.new_handle(), then the Python object has already been garbage collected
                                                                                                          
Current thread 0x00007dca243f6700 (most recent call first):                                               
  File "/home/gregs/code/pants/src/python/pants/engine/native.py", line 293 in extern_get_union_for
as far as I can tell I'm using my new 
extern_get_union_for
function in the same way as other python extern functions
the 
context_handle
is being provided by the same 
with_externs
helper that every other rust function in 
<http://externs.rs|externs.rs>
is using
am I doing anything obviously wrong?
a
You shouldn’t need to do this, ever. Any time you do, please find out why and fix it.
any time i try to describe RTB we really need to be doing, including specifically with respect to the rust build, it gets shot down, so i get to do it on my own time. i did say specifically it was a huge hack in order to avoid creating the impression that we think this is ok.
sorry about that
i'm looking now
this is failing at the first 
from_handle()
call in that method?
what is the full stack trace?
the first thing i see is that no other extern is receiving anything complex except for a 
Handle
seeing why that would be an issue
h
there's at least one other one that's receiving a TypeId
a
got it
receiving a TypeId?
h
yeah 
extern_type_to_str
a
i am silly yes
can you provide the pants command line and full output? just bootstrapping pants isn't reproing
h
what pastebin does this project use?
a
that is a question i have never heard before! we are based on github so gist seems appropriate
h
a
ok, got distracted, but i repro. investigating
h
cool
is it possible that the *const ExternContext pointer is invalid because I'm calling this in error handling?
a
that sounds very reasonable, and i am paging into what the extern context is exactly
ah
the type is possibly not wrapped correctly (even though i see no obvious issues)
the explanation why this is only breaking for your method is possibly because type_to_str hasn't been used in this code path yet. seeing if that's true
h
so, maybe type_to_str would break in the same way if used here?
a
yes, although i don't think i'm seeing that. adding another 
eprintln!()
to 
gen_get
rn
hm. it appears to be failing specifically at this
with (in 
gen_get()
):
Copy code
let declared_type_union = get.declared_subject
                .and_then(|ty| {
                  let t = externs::type_to_str(ty);
                  eprintln!("this type??? {:?}", t);
                  externs::get_union_for(ty)
                });
              eprintln!("declared: {:?}", declared_type_union);
i am seeing:
Copy code
this type??? "TestTarget"
`
then the same fatal python error immediately after
h
so type_to_str works but not get_union_for
a
it seems so
h
hm
I'm gonna add some prints in the python to see exactly where that fails
a
good call
the context you mentioned might be it (we can get a write lock on it using 
with_externs_exclusive
, i'll try that next although i don't think that's it). it might also be because (and this would be ridiculous) the cffi-generated C in 
native_engine.c
has this conditional based on 
sizeof()
which might be different
i'm seeing it fail at the very first line 
c = self._ffi.from_handle(context_handle)
the backtrace gives line numbers
h
yeah same
oh so maybe I need to write my wrapper around the extern differently?
a
that would be the first idea, only because i happened to spy a 
with_externs_exclusive()
method and since we happen to have a bad context maybe a write lock would help
h
where's this conditional in native_engine.c?
a
in the generated file in 
src/rust/engine/src/cffi/native_engine.c
, there's:
Copy code
static struct _cffi_externpy_s _cffi_externpy__extern_get_type_for =
  { "native_engine.extern_get_type_for", (int)sizeof(TypeId) };

static TypeId extern_get_type_for(void * a0, void * * a1)
{
  char a[sizeof(TypeId) > 16 ? sizeof(TypeId) : 16];
  char *p = a;
  *(void * *)(p + 0) = a0;
  *(void * * *)(p + 8) = a1;
  _cffi_call_python(&_cffi_externpy__extern_get_type_for, p);
  return *(TypeId *)p;
}

static struct _cffi_externpy_s _cffi_externpy__extern_get_union_for =
  { "native_engine.extern_get_union_for", (int)sizeof(OptionalTypeId) };

static OptionalTypeId extern_get_union_for(void * a0, TypeId a1)
{
  char a[sizeof(OptionalTypeId) > 16 ? sizeof(OptionalTypeId) : 16];
  char *p = a;
  *(void * *)(p + 0) = a0;
  *(TypeId * *)(p + 8) = &a1;
  _cffi_call_python(&_cffi_externpy__extern_get_union_for, p);
  return *(OptionalTypeId *)p;
}
and like idk if 
sizeof(OptionalTypeId)
happens to be greater than 16, or if that's different compared to 
TypeId
i do not expect this to be the issue right now
also going to google for this error
h
using with_externs_exclusive doesn't prevent the error
a
yes, i also found that
i tried printing out the value of the 
context_handle
before calling 
.from_handle()
for 
type_to_str
and the subsequent 
get_union_for
, and it is different
but that might be 100% expected if i knew more about cffi
i need to switch to something else now but basically: (1) after applying https://pantsbuild.slack.com/archives/C0D7TNJHL/p1561582043101900?thread_ts=1561163619.038700&amp;cid=C0D7TNJHL (2) adding 
<http://logger.info|logger.info>('h: {}'.format(context_handle))
to the top of 
extern_get_union_for
(3) adding 
<http://logger.info|logger.info>('i: {}'.format(context_handle))
to the top of 
extern_type_to_str
(4) running 
RUST_BACKTRACE=full ./pants --no-v1 --v2 test examples/src/java/org/pantsbuild/example/hello/simple
(5) i see
Copy code
21:16:15 [INFO] i: <cdata 'void *' 0x110d02728>
21:16:15 [INFO] Starting tests: examples/src/java/org/pantsbuild/example/hello/simple
21:16:15 [INFO] i: <cdata 'void *' 0x110d02728>
this type??? "TestTarget"
21:16:15 [INFO] h: <cdata 'void *' 0x70000dcb9d58>
Fatal Python error: ffi.from_handle() detected that the address passed points to garbage. If it is really the result of ffi.new_handle(), then the Python object has already been garbage collected

Thread 0x000070000d4b4000 (most recent call first):

Current thread 0x000070000dcc0000 (most recent call first):
  File "/Users/dmcclanahan/tools/pants-v3/src/python/pants/engine/native.py", line 294 in extern_get_union_for
(6) turning the 
self._ffi.from_handle()
to use 
.new_handle()
fails with 
illegal hardware instruction
(7) mainly, i do not understand why the context object would be different when passed to 
extern_get_union_for
(as shown in the above output) immediately after being passed to 
extern_type_to_str
and there appear to be no successful invocations of 
externs::get_union_for
whatsoever
h
right, 
get_union_for
is the new function that I'm adding with this commit, meant to be used right here
2 Views
Open in Slack
PreviousNext
Powered by