<headius>
Matt Welke: it might be in jruby-base now
<headius>
we restructured release artifacts so that jruby-base is just JRuby itself, jruby-core is a shaded version of jruby-base with all Java dependencies, and jruby-complete is jruby-core plus the Ruby libraries
<headius>
I just triggered it to process jruby-base so it should show up there soon
sagax has joined #jruby
<MattWelke[m]>
nice, ty. I'll be looking at the Java docs this weekend if I play around with the Java extension stuff we talked about earlier this week.
<MattWelke[m]>
I see base, but not core or complete yet. Looks like it's working in the background.
<headius>
yeah those other two probably won't have any docs
<headius>
core just adds third-party libs to the jar and complete adds Ruby libraries that we don't generate docs for
<MattWelke[m]>
oh true
<MattWelke[m]>
then I'm all set
<MattWelke[m]>
When I click the link from the website, it brings me to https://javadoc.io/doc/org.jruby/jruby which you're saying, will no longer show anything, right?
<MattWelke[m]>
I have to be aware that the docs are in jruby-base and change the dropdown to that to see the docs.
<headius>
jnr releases are out, I'm going to start merging things around
<enebo[m]>
ok
<enebo[m]>
I can see where we go off the rails on prepend+include example
<headius>
so to speak
<enebo[m]>
we gather modules to include and when it gets to the prepended Enumerable it will say nah give me the real one
<enebo[m]>
the real one has no methods in it
<enebo[m]>
so this should not have anything to do with JI yet I have not done the proper magic yet to repro outside of it
<enebo[m]>
gatherModule intentially skips prepended modules to get the real one
<enebo[m]>
So I will quickly look but it is possible we are just not setting the included wrapper to get the proper method table
<headius>
that would be a nice simple fix
<enebo[m]>
Ah maybeing it should be getMethodLocation().getMethodForWrite!
<headius>
ah yeah probably, since pre-3 we just want the module'
<enebo[m]>
That will be funny if so since I have been literalling getMethodLocation everywhere
<headius>
module's own method table
<headius>
we don't care about other things in its hierarchy because we gather those separately
<enebo[m]>
I think for 3 this is largely recording each of these wrappers updating them if anythign changes but those wrappers will get recorded in real module
<enebo[m]>
which may be what you are saying
<headius>
I was under the impression that they only have one wrapper but search the other hierarch when they get to it
<headius>
rather than copying everything and trying to keep them in sync
<enebo[m]>
I saw something which looked like it maintained a list
<enebo[m]>
so methodlocation did not fix it directly
<enebo[m]>
I should see what else happens now
<headius>
if they are just tracking the original module better now then it will be a simpler change for us
<headius>
yeah that seems like too many things 😀
<enebo[m]>
I am just happy I made some progress
<enebo[m]>
It is becoming clearer we are saying "include the real thing" but the real thing has gotten its methods replace with no methods because the prepended shim has them
<headius>
right
<headius>
usually that is the problem... still have code around not aware of prepend and not going to the new location for method table
<enebo[m]>
Seems like real thing getMethodLocation should point to that so that is the confusion
<enebo[m]>
yeah we still have a number of places but some are pretty primal where we are setting up the type
<enebo[m]>
In my mind the optimization of not doing getMethodLocation in those cases are not worth it because this is too fragile
<enebo[m]>
This fix breaks a single refinement spec which has a prepent
<enebo[m]>
but I will restep through this. One thing I am starting to remember is one thing I "fixed" which did fix things was we were unconditionally passing realMod.getOrigin() into the wrapper instance
<enebo[m]>
I can still see it in deprecated cases
<enebo[m]>
we now pass in realMod.getDelegate() and that method is pretty unclear to me
<enebo[m]>
Seemingly we only call it for included modules wrappers to get the proper module
<enebo[m]>
So it is possible for prepends we maybe do something with getDelegate there as well
<enebo[m]>
One more time through the debugger....
<enebo[m]>
I was wrong. getOrigin wasn't needed there at all
<enebo[m]>
Not sure what happened. Maybe the debugger did not pick up the change when I restarted
<enebo[m]>
The fix actually makes sense but now I have to hope that refinment error dropped by removing the getOrigin
<headius>
yeah I didn't think getOrigin was needed either
klobuczek[m] has quit [Quit: You have been kicked for being idle]
<enebo[m]>
It can only make sense if the included wrapper receives an origin that is not the actual module
<enebo[m]>
but we call realModule.getDelegate() which seems to only be different is we are including a module wrapper
<enebo[m]>
So I am wondering if getDelegate actually does anything for this
<headius>
these names are so confusing
<enebo[m]>
yeah
<enebo[m]>
I did add a nice comment for getOrigin
<enebo[m]>
I believe I also removed all usages of getNonIncludedClass too
<enebo[m]>
but getDelegate :)
<enebo[m]>
This fix is obvious after the fact but it might make us realize a deficiency in our refinement impl
<headius>
git st
<headius>
blah
<enebo[m]>
<some git output>
<headius>
I am using an external monitor lately so I keep getting confused
<enebo[m]>
That was my 'b' to you yesterday...I did C-X b
<enebo[m]>
Luckily pidgin has no binding for control-x
<headius>
oh hah
<enebo[m]>
So this fixes the problem but introduces another where a refined method is called instead of a prepended one
<headius>
hmm
<headius>
pull one thread and another comes unraveled
<enebo[m]>
which I guess would mean getOrigin on something refined maybe is not getting the prepended stuff but the refined stuff
<enebo[m]>
err sorry not origin
<enebo[m]>
getMethodLocation
<enebo[m]>
I am going to start a branch on this since this is the right fix as it stands
<enebo[m]>
We 100% were just looking at the prepend removed methods version of the module
<enebo[m]>
I need to update the dates on the website before I push
<headius>
its merged and I will merge 9.2 to 9.3 etc now
<headius>
and make sure 9.2 java changes for interruptibility don't land on 9.3
<enebo[m]>
I am going to push site
<headius>
I had to do a squashed merge to avoid the java changes, but there's very few commits merging over
<headius>
for 9.2.20.1 yeah?
<headius>
and we still need to figure out refinement thing for 9.3.2?
<enebo[m]>
yeah it was pushed yesterday except announcements
<enebo[m]>
yeah 9.3.2 should have this fixed
<enebo[m]>
And it would be nice if we didn't fix one thing to regress another
<headius>
ok everything is merged and pushed up to 9.3
<enebo[m]>
so I am hoping to figure this out today and we can talk tomorrow on how we feel
<headius>
I will wait on merging to master until we have 9.3.2 ready
<enebo[m]>
The regression fixed feels worse than what broke (refinements with a prepend in it)
<headius>
should I not be worried that we'll have the date fix out for 9.2 but not 9.3?
<enebo[m]>
err that was ambiguous...what I just fixed is more important than what it just broke
<headius>
I just keep feeling like it is saying we patched this security issue in 9.2.x but you have to wait until tomorrow for the 9.3.x fix
<enebo[m]>
nope
<headius>
I agree with that
<enebo[m]>
We will fix it soon but people already can know about the CVE
<headius>
the prepend enumerable thing has broken a lot of people
<enebo[m]>
so they are no worse off but if we release tomorrow it will be fine
<enebo[m]>
I think most people are able to judge how important the date thing is too
<enebo[m]>
but my position is we are already vuln in both so fixing one is better than holding off to get both out at same time
<enebo[m]>
If we cannot figure out this regression by tomorrow we can also just put out a security fix too but I think this regression will affect more people than the CVE
<headius>
yeah
<enebo[m]>
HAHA I was like wtf...how could our site have changed since I pulled it yesterday but I see you fixed the javadocs
<enebo[m]>
My first reaction was who the hell is hacking our site
<headius>
oh heh yeah that was just me
<enebo[m]>
headius: will you tweet + socials + docker + rbenv
<enebo[m]>
I am emailing and will github release and notify release people
<headius>
yeah you have my notes on the security release?
<enebo[m]>
yeah I am ok I think with switching the regressions but I would like to spend a few hours looking at it
<enebo[m]>
well hopefully less but that is my time box
<headius>
it's possible the failed spec was fixed by the broken change
<enebo[m]>
I think prepending in an refinement must be uncommon?
<headius>
well I sure hope so
<headius>
your PR is not based on 9l3
<headius>
9.3
<enebo[m]>
so yeah I am not concerned about shipping it but it would be nice to figure out why
<enebo[m]>
ah yeah I did not toggle that
<enebo[m]>
I will change it
<headius>
huh how did you create this PR? It has your commit text indented like a quote
<headius>
docker PR failed and doesn't look like my fault
<headius>
error fetching some busybox image
<enebo[m]>
I opened it using the URL github provides in the response
<enebo[m]>
then I copy/pasted my commit message but I realize what happened
<enebo[m]>
it is indented
<headius>
yeah
<enebo[m]>
I was doing this quickly
<headius>
ok just was curious
<headius>
I make PRs the same way
<enebo[m]>
I must usually copy the comment from the PR itself? I am a little confused because I do this same pattern quite a bit
<enebo[m]>
Maybe I manually correct the indentation but I don't recall ever doing that
<headius>
what is the spec that fails?
<enebo[m]>
/home/enebo/work/jruby/spec/ruby/core/module/refine_spec.rb:319:in `block in <main>'
<enebo[m]>
as far as descriptions goes this is extremely straight-forward
<enebo[m]>
the prepend overrides the refined version of a method
<enebo[m]>
We are just one getMethodLocation away
<headius>
ok nothing has changed in these tags recently
<enebo[m]>
using seemingly copies that refinement blobber and we end up ignoring the prepend so getMethodLocation is now not finding the prependedmodule first
<enebo[m]>
or ever
<headius>
yeah I am looking into Module#using
<enebo[m]>
another extra early lunch(tm)
<enebo[m]>
should be pretty quick though
<headius>
I have your branch and will try some things
<headius>
the refinement logic uses getOrigin where it might want to be using getDelegate, unsure if that's it though
<headius>
ah I see why it breaks and it conflicts with the fix
<headius>
by getting the method location in the IncludeModule constructor, we walk away from the prepended bottom class
<headius>
so the fix is the break
<headius>
er, I mean in IncludedModuleWrapper constructor
<headius>
hmmm
<headius>
but this should be getting the module that was prepended, not the prepend wrapper
<headius>
enebo: they do not appear to dig out method location in rb_include_class_new so maybe we do need to move it outside of the IncludedModuleWrapper constructor
<headius>
I have an alternative fix
<headius>
eh you did not link bug in your commit message
<headius>
I'm not sure which example to use to retest this but here's the diff
<headius>
I moved the method location call out to the place where we finally do the include/prepend of the module, rather than in the constructor for the wrapper
<headius>
I think it should still fix original issue without breaking the refinement
<enebo[m]>
hmm
<enebo[m]>
This ends up changing origin in the wrappen and adds this wrapper to the methodlocation and not the original module
<enebo[m]>
I half wonder if we are trying to do two things here is includedModuleWrapper
<headius>
yeah that's why it seemed more right to move it out of there
<enebo[m]>
with your patch in the include we will get getMethodLocation and use it as the origin which will be a non-origin wrapper but in the proceedWithInclude it is still passing in the origin
<headius>
hm
<enebo[m]>
So I think passing in a non-origin here is wrong but only because it means we are not dealing with the actual module with this change and that is more confusing to me
<headius>
we could pass both in
<headius>
but I am throwing darts here
<enebo[m]>
yeah but if we setOrigin to be the real module will your patch continue to work?
<enebo[m]>
err wait...I need to actually apply this.
<headius>
setOrigin where?
<enebo[m]>
I thought I knew the two places but I realize now the usage of this wrapper is wider
<enebo[m]>
it is in the superclass of IncludeModuleWrapper
<enebo[m]>
we pass origin down from its constructor
<headius>
hmm yeah I am not sure what origin is used for from the wrapper
<headius>
only failure on my PR looks like a flaky spec
<headius>
signal-related
<enebo[m]>
You fix only works I think because you change origin
<enebo[m]>
Or actually the second one is that all the refinement code also uses this and getMethodLocation in those places might get screwed up
<enebo[m]>
I would be curious if we ever see origin not be the real module without the patch
<enebo[m]>
I think my fear is this breaks the mental model I have for what origin is.
<headius>
my fix works because it does not get the method location from the prepended refinement
<enebo[m]>
If so then we could pass origin and get method location it needs to go right before we make the wrapper?
<headius>
when we pass in a refinement module that has a prepend to this wrapper it needs to use literally that refinement module or we get that failure
<headius>
so I figured the include wrapper should not be doing the digging itself, it should just wrap what is is given
<headius>
include should dig because it needs to just get the original method table for the module since we gather the prepends and includes separately
<headius>
so I moved it out to there
<headius>
but I have no explanation for how origin should work
<headius>
what about MRI?
<enebo[m]>
yeah origin is my hangup because whatever calls getOrigin will get a wrapper instead of the real thing
<headius>
when they include, do they use the actual module object or the prepend redirect module as the origin?
<enebo[m]>
we could pass in them separate and see if anything breaks
<headius>
not a wrapper
<headius>
oh well yeah a wrapper
<headius>
but it is the wrapper used to house the relocated method table
<enebo[m]>
include will skip prepend roots and use the real thing. include itself starts with the real thing
<enebo[m]>
So origin seems like it is the real module
<headius>
so yeah I am leaning toward agreeing that the origin should remain the module object and not the relocated wrapper but I am guessing
<enebo[m]>
but your suggestion to pass in the methods works for me if things pass
<enebo[m]>
That is the essence of the original fix
<enebo[m]>
and it is done in the constructor with my fix but if it is passed in nothing changes except that...just where it calls it changes
<enebo[m]>
I am very confident about getMethodLocation fixing the prepend issue
<enebo[m]>
So I will just add a parameter for methods to constructor but still pass in origin
<headius>
getMethodLocation does fix prepend issue in both patches
<enebo[m]>
yeah
<enebo[m]>
so you have a second fix here
<enebo[m]>
the raw getMethods for adding the refinements
<headius>
yeah it seems like it was using the wrong table
<enebo[m]>
That will literally be wrapper.getMethods()
<enebo[m]>
Or it could be right?
<enebo[m]>
since we are passing that in
<headius>
what do you mean
<enebo[m]>
the pr is methodLocation.getMethods() but we pass methodLocation in so the included wrappers methods is the same thing
<enebo[m]>
then the ifRefinement() block is two statements looking at or modifying the wrapper
<headius>
so you think we should pass the original origin but add the methods when calling IncludedModuleWrapper constructor>
<enebo[m]>
I think so.
<headius>
hehe
<enebo[m]>
Here is one fear I have with my original PR and also with this second one
<enebo[m]>
We have refinments using this modulewrapper all over and it is likely passing the real origin in
<enebo[m]>
I think just from a delta of changed behavior keep origin be the same as it was is a good idea
<headius>
I'm looking at the CRuby include logic for a sec
<enebo[m]>
It also has the benefit that it always seems to really be the origin (or original module)
<enebo[m]>
Adding a second constructor for origin + methodLocation and only using it in this one place is maybe more of a bandaid than a proper fix (who knows?) but it does limit our risk
<headius>
hmm
<enebo[m]>
I will commit to my branch what I mean
<headius>
yeah ok
<enebo[m]>
but look at constructors to includedmodulewrapper
<headius>
the C code is not helping me
<headius>
yeah I know what you mean to do
<enebo[m]>
we use is a number of places for refinements
<enebo[m]>
ok
<enebo[m]>
who knwos this may break something?
<headius>
I am just trying to suss out what MRI does
<headius>
they get origin of the module passed into include
<enebo[m]>
yeah and my belief is that means Enumerable
<headius>
that goes into include_modules_at and then they get the methods directly from it
<enebo[m]>
but maybe that is why we have problems :)
<enebo[m]>
getOrigin also returns origin but I am wondering if they are both always the same
<enebo[m]>
in my last fixes for prepend I removed getNonIncludedClass for getOrigin since they were 100% always the same value
<enebo[m]>
but things went from getDelegate().getNonIncludedCModule -> getDelegate().getOrigin()
<enebo[m]>
I would have killed origin or delegate had they always agreed...or at least I feel I would have
<headius>
there is also this in the MRI loop walking all modules to include:
<headius>
if (RCLASS_ORIGIN(module) != module)
<headius>
goto skip;
<headius>
I believe this will skip when it sees include wrappers so it ends up walking up to the method location as a result
<headius>
because that one has the right method table
<enebo[m]>
yeah we do have something like that almost...we only skip if it is a prepend root (e.g. prependedmodule)
<headius>
using the definition I think MRI uses
<headius>
try pushing your fix?
<enebo[m]>
I submitted the link above
<headius>
digging out the methods in the constructor just breaks the refinement thing because it doesn't see prepends then
<headius>
so I think moving it out is better and we can debate origin without changing it in this release
<headius>
we will probably find some tagged specs that break because of origin
<headius>
ok
<enebo[m]>
we do have delegate checks and not origin checks in our doIncludeModule logic which does the skip
<enebo[m]>
I would not be surprised if our logic was approximately the same as their in 2014
<headius>
I'm fine going with your patch if it passes everything , since it reduces the risk changing origin
<enebo[m]>
Am I crazy or is the GHA UI links into and out of actions stuff impossible to find
<headius>
You mean the details link in the list of jobs?
<enebo[m]>
I keep searchign around with my mouse trying to find where I can see the whole list of items vs just one
<enebo[m]>
if I go up to Actions it is easy to get to that point
<enebo[m]>
if I am in actions and I want to go to the commit which kicked this off where is that?
<enebo[m]>
Or the PR maybe
<headius>
Oh yeah I'm not sure if there's a way to navigate from the pull request to the overall workflow
<headius>
Not sure about the other way around either
<headius>
It is not as obvious as with Travis
<enebo[m]>
I think CI will be like buld tools...always maligned
<headius>
So I think we agreed that we will not be releasing 9.3.2 today, right ?
<enebo[m]>
Well I am happy to do it in the morning but it looks like this is green
<enebo[m]>
waiting on spec:ruby:slow
<headius>
Have you run your verifications on this yet?
<headius>
I'm fine releasing either day but it's already after 1:00
<enebo[m]>
I put a FIXME in that commit saynig we should evaluate the original constructors that refinments uses to make sure we could maybe always just do the same thing
<headius>
Ok
<enebo[m]>
yeah I will spin bits this afternoon and kick the tires
<headius>
I am remembering adding a comment saying that origin is not the same as in c Ruby but I can't find it now
<enebo[m]>
If this breaks Rails we will have some time to figure that out
<headius>
It may have just been in a commit that fixed something related but I know our names are not referring to the same thing somewhere in here
<enebo[m]>
tbh I don't feel great about any changes to refinements
<enebo[m]>
I do think prepend fix is correct though.
<headius>
Yeah this doesn't change anything in refinements, it just relocates your fix so that it doesn't interfere with the refinement seeing the pre-pended layer
<headius>
So it localizes your fix to just the include
<enebo[m]>
you will see I do wrapper.getMethods there since it will be same value
<headius>
I did notice that
<headius>
Hopefully that will always be the case 😀
<enebo[m]>
well it is an object we just made but I suppose in 3-4 years we may add another one in between those two points :)
<enebo[m]>
or maybe do serious magic in the constructor
<headius>
I blame Ruby for having an overcomplicated class system
<enebo[m]>
I do hope the 3.0 work clarifies this a bit more in my mind
<enebo[m]>
having to update existing uses to remove the temporal nature will probably require staring at this crud more
<headius>
Yeah we still need to get on that. I don't think our timeline will be too far behind but 9.4 is not going to be ready by the end of the year
<enebo[m]>
no. and I am planning on next week off still
<enebo[m]>
winter break will happen as well
<headius>
Core classes and standard library should be solid but we have a number of language things to iron out
<enebo[m]>
with that said we are ~200 F/E on each suite and that is mostly overlap
<headius>
And we can't really finish greening up to suites until we fix the language stuff
<enebo[m]>
40 of those are Time#{ceil,floor} not existing
<headius>
Yeah that's a weird one
<enebo[m]>
like 75% are not much more than labor...like the new warning subsystem needs to switch on category
<enebo[m]>
StrScanner needs to work with strings/regexps
<headius>
I still have a few extensions that need to move into MRI gems
<headius>
But we are getting there
<enebo[m]>
which is probably more work than the other two but you see what I mean. We have a lot less work other than fixing kwarg arg bindings AND this prepend/include stuff
<enebo[m]>
Once those two things are done we really will be just about there
<headius>
I will try to revisit my keyword argument stuff the rest of this week so it stays fresh in my head but I think it's on the right track
<enebo[m]>
yeah once that is working I think we will see the weird binding bugs disappear
<headius>
The way we mark that a hash is from keywords might be a little cumbersome right now
<enebo[m]>
I tried a stab at Fiber#raise and that got rid of 15 or 18 or so fails but I have no confidence because after the raise the fiber was still usable :)
<enebo[m]>
That literally took about 5 minutes but I am pretty sure it is partially wrong because I just called Thread#raise internally :)
<headius>
They are moving more and more towards a fiber behaving like a thread
<headius>
Okay I need to step away for a while
<enebo[m]>
ok
<headius>
Was plan on release tomorrow and you can do whatever verification you need
<headius>
Let's plan
<headius>
jnr update and everything else is merged into 9.3 9.3
<enebo[m]>
yeah
<enebo[m]>
if things look good this afternoon let's plan on releasing morningish
<enebo[m]>
headius: when you are next on perhaps look at reducing the targetted 9.3 issues. We can punt for 9.3.3 but we seem to punt a lot without fixing them