-
Notifications
You must be signed in to change notification settings - Fork 278
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
Move from label collection to label stamping #839
Comments
@long-stripe wdyt about taking this one? Shouldn't be very complicated and will be a nice win (remove a lot of noise from our code) |
@ittaiz Yeah if you think it's a good thing to start on! I can't promise anything timeline wise, but I will try and carve out some personal time in the coming week or so to look at this. |
I do :)
Re timeline it's ok- we've been wanting to do this for about a year so
another month or two won't probably change anything...
…On Mon, Sep 2, 2019 at 8:25 AM Long Cao ***@***.***> wrote:
@ittaiz <https://github.com/ittaiz> Yeah if you think it's a good thing
to start on! I can't promise anything timeline wise, but I will try and
carve out some personal time in the coming week or so to look at this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#839?email_source=notifications&email_token=AAKQQF6RQKCINMLWQ7U2JL3QHSPTTA5CNFSM4ISZLEAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5UXSCY#issuecomment-527005963>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKQQF7IBQFSFPUVKAFIKGLQHSPTTANCNFSM4ISZLEAA>
.
|
FYI small heartbeat - I am still interested in tackling this, but haven't had time lately. |
Looking forward to it. You're not waiting for something from me, right?
…On Fri, Sep 20, 2019 at 8:28 AM Long Cao ***@***.***> wrote:
FYI small heartbeat - I am still interested in tackling this, but haven't
had time lately.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#839?email_source=notifications&email_token=AAKQQF4NJHXI2Y3VPAFFY6DQKRNO3A5CNFSM4ISZLEAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7FTL3Y#issuecomment-533411311>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKQQF4KQYUPPBVS3OJL6ZTQKRNO3ANCNFSM4ISZLEAA>
.
|
@ittaiz I was just gonna meander about and deep dive in when I have the time, but if you have links to relevant parts of the codebase that I should be focusing on, that would be super helpful! |
Unfortunately it’s spread throughout the codebase. I’d start in the lead (compiler plugin) and trace back. |
Currently we collect labels in various places (scala_library/scala_binary/scala_import/etc) and propagate them onwards.
This is valuable since when we want to report to the user on a problem with a transitive dependency (unused deps/strict deps) we have a meaningful label and not just a file path.
Java rules also used a similar (but different) approach but have moved a year or more ago to a different approach.
Their current approach "stamps" the jar upon creation* with its label (k/v in the manifest.mf) and so if they ever need to report (which is the rare case) then and only then they pay the (higher) price of opening the jar and reading the manifest to improve the user reporting.
This doesn't actually happen on jar creation but implicitly on ijar creation.
This means that we need to explicitly stamp our outputs for scala_import and scala_macro_library since they don't go through ijar. java_common has a special API for this.
Note that some tests will fail with this change since our existing mechanism allows us to remap labels (if
bar
exportsfoo
then when someone usesbar
the outputs offoo
will be listed asbar
). This is valuable in having facade targets for logical libraries.After lengthy discussions it was agreed that the solution for this pattern should be in a smarter tool that will use bazel query to find the fact that
bar
exportsfoo
and will addbar
to the caller.Also the changes in #716 are needed.
The text was updated successfully, but these errors were encountered: