daveg_lookout[m] has joined #jruby
<daveg_lookout[m]> Hash#slice() doesn't work with HashWithIndifferentAccess, since the former is implemented at the Java level and ignores the method aliasing from the latter. Is this something worth submitting a bug over, or is it known/won't fix? I haven't tried on 9.3.4.0, but it doesn't look like code has changed since it was first implemented
<enebo[m]> daveg_lookout open an issue. Even if this is something we wontfix (and I am not saying that) we will document it more for anyone else who looks for it. This sounds like we are doing a java direct call instead of dynamic dispatch.
<enebo[m]> headius: I just nuked all my gems and I am starting over on generating a rails app in case I somehow changed something. I definitely made it past the delegation thing
<headius> Hmm ok
<enebo[m]> Successfully installed net-pop-0.1.1
<enebo[m]> Building native extensions. This could take a while...
<enebo[m]> headius: Am I still depending on a prerelease of a gem?
<enebo[m]> I was really heavy handed and nuked all gems/gemsspecs and did a full rebuild
<daveg_lookout[m]> Just finished testing it while writing up a ticket, looks like this was a bug in ActiveSupport somewhere between 6.0.2.1 and 6.1.5). I'll try to find activesupport bug to make sure. Sorry!
<enebo[m]> daveg_lookout: no problem
<enebo[m]> rails is a bigger and bigger piece of software. So is Ruby for that matter :)
<enebo[m]> Ah yeah I did --pre to get strscan installed and get rails to not complain about wanting 3.0.1
<enebo[m]> I don't recall what the second gem was but I guess I made it past gem install point for rails with that
<headius> Yeah I can install all the gems okay but generating an app fails
<enebo[m]> bleh now I realize we have no arjdbc-7
<enebo[m]> I will have to add that to the list for this week since it installs 5
<headius> ah shoot, yeah
<enebo[m]> I am a bit confused how my previous rails app bundle resolved to arjdb 50.0 and worked
<enebo[m]> It doesn't matter. I think I will just make a db-less app now anyways since our problems are not related to that currently
<enebo[m]> once rails loads then I can see fitness of arjdbc7. I also realize this is likely still a travis project
<headius> same result generating with --dev here
<headius> thought maybe that was the missing sauce
<enebo[m]> headius: are you using JRUBY_OPTS for that?
<headius> yes
<enebo[m]> require 'maven/ruby/maven'
<enebo[m]> What gem is this from?
<enebo[m]> I am trying to build a 3.0.2 of strscan to just not deal with --pre
<enebo[m]> ruby-maven
<headius> ruby-maven
<headius> I attached pre gems to that Rails 7 issue
<enebo[m]> this maybe should be in strscan's gemspec for development unless we ship it
<headius> you can just install them and then install works normally
<enebo[m]> ok I forgot those were attached there
<enebo[m]> Those two gems are even sitting in my downloads dir
<enebo[m]> Took me a while to get it to install but I am seeing this
<headius> you got the app to generate?
<enebo[m]> somewhat :)
<enebo[m]> It was complaining about wanting a specific version of jdbc-sqlite3 which it wasn't doing so I manually uninstalled the newer version so only that was there
<enebo[m]> I will try this again though with skip ar option
<enebo[m]> since then it should be clean
<enebo[m]> rails aborted!
<enebo[m]> NoMethodError: private method `importmap=' called for #<Testv::Application:0x68c615f3>
<enebo[m]> /home/enebo/work/jruby/testv/config/environment.rb:5:in `<main>'
<enebo[m]> Tasks: TOP => app:template => environment
<headius> aha
<headius> hmm monitor.rb
<enebo[m]> So this is with my monitor fix since ... + zsuper is still broken
<enebo[m]> I have to fix this since it should work but that fixes it enough for us to see this import map issue
<headius> ok
<headius> that super thing should be an easy fix no? Just compile it like super(...)?
<enebo[m]> I started putting in send as an experiment but this importmap is called from a number of places
<enebo[m]> no it is not simple
<headius> show me your session with app generating etc so I know how to repro your setup
<enebo[m]> if it was purely (...) and (...) I could possibly detect that much and bypass how we build up a zsuper
<enebo[m]> you can do stuff like def foo(a, ...); super; end
<headius> sure
<enebo[m]> but it is possible I can detect ... but the way zsuper works is we extract all references to receives so we can get the proper variable and we build a call out of those extracted things
<headius> you could just commit the monitor fix for now if you want... we are not pulling this from the gem
<headius> so we'd differ slightly but it will go back once fixed
<enebo[m]> so that extraction would have to then ignore those receives
<enebo[m]> I think the zsuper logic just needs to extract those values and I have to add the keyword arg marking++ that ordinary calls so
<enebo[m]> s/so/do
<enebo[m]> It is pretty icky
<enebo[m]> but I will commit monitor for now
<enebo[m]> That has been pushed
<enebo[m]> In theory I think you should be able to run the command line in the gist above and get it generating until it hits importmap stuff
<headius> yeah that is where it fails for me too
<enebo[m]> once you do get to this point I think you can just try rails console and it will be a short repro
<headius> getting a clean build and I will try it
<enebo[m]> app.importmap{,=} may be some funky delegation
<enebo[m]> but I do know self.private_thing is now just private_string and works. I did fix that feature but perhaps there is more to it than just self.
<enebo[m]> It was always weird that self. on front of private method would not work
<headius> so you get that importmap thing during generation as well
<enebo[m]> yeah it will be the end of that gist
<enebo[m]> It is part of rails importmap:install
<enebo[m]> amusingly the backtrace is just points to: Rails.application.initialize!
<enebo[m]> Disabling all private errors in call would tell us if that is the only thing holding us from running though
<headius> someone filed it last fall
<enebo[m]> yeah Ruby 2.7 changed visibility in some way where this works
<headius> # Use Rails.application.importmap to access the map
<headius> Rails::Application.send(:attr_accessor, :importmap)
<headius> oops
<headius> # Use Rails.application.importmap to access the map
<headius> Rails::Application.send(:attr_accessor, :importmap)
<enebo[m]> but this should work for us since we are >2.7
<headius> this is in the importmap engine.rb
<headius> it is likely using top level visibility for the send(:attr_accessor ...) which would make it private
<enebo[m]> ah so maybe that is no longer the case in 2.7+
<headius> yeah possibly
<enebo[m]> we do have some top-level errors in specs but I thought it was mostly stuff you couldn't do
<headius> I will investigate this
<enebo[m]> this would be something you can now do :)
<headius> can I push to your branch or do you want to merge it?
<enebo[m]> yeah coolio. I am hoping this is last thing to getting it started
<enebo[m]> I think we can I noticed spec:ruby:fast:jit has 3 errors
<enebo[m]> but we can just address them after I merge
<headius> trivial repro
<headius> class Foo; end; Foo.send(:attr_accessor, :foo); Foo.new.foo
<enebo[m]> Also spec:compiler is still broken but that is also ok
<enebo[m]> oh sweet
<enebo[m]> send changes
<enebo[m]> headius: merged to master.
<enebo[m]> I will fix persistence next so spec:compiler is ok
<enebo[m]> then since monitor is worked around I will see what the new failure is in keywords and also the JIT errors involving keywords
<enebo[m]> going to take a break to get some fresh air
<headius> ok
<headius> I need to trace the lineage of this but at some point they stopped looking at frame visibility if the cref class is not the self class
<headius> so this covers all send cases... they should default to public if you do it from outside the class itself
<headius> that is the localized fix... the other attr forms need this change too if it is right
<headius> I am verifying it
<enebo[m]> I haven't quite went afk...I am also trying this patch
<enebo[m]> LoadError: load error: /home/enebo/work/jruby/testv/config/environment -- java.lang.NullPointerException: null
<enebo[m]> ok something else
<headius> 17) Error:
<headius> TestModule#test_attr_public_at_toplevel:
<headius> so yeah there were definitely changes here
<headius> aha and this would have failed on attr_accessor first but I fixed that one
<headius> mmm nice
<enebo[m]> yeah so a quick guess is splatting on an invisible variable but that should still have a dynamic scope value
<enebo[m]> so something
<enebo[m]> I will look into this next I guess
<headius> yeah
<headius> ok
<headius> I am applying this fix elsewhere and confirming it does not regress things
<enebo[m]> remember yesterday I said eliminating **{} from callside meant looking at splat, array, args{push,cat}
<enebo[m]> So this may be related to that perhaps since it is a splat...
<enebo[m]> I am still bullish on this working today
<enebo[m]> we do not have too much left
<enebo[m]> once this loads I can then move over ar to use GHA and then run rails 7 on master
Guest51 has joined #jruby
<enebo[m]> maybe dr-itz will be helpful too
<enebo[m]> bbiab
<headius> full fix passes this test
Guest51 has quit [Quit: Client closed]
<headius> enebo: I do not have rights to push to your branch
<headius> pushed to headius_new_kwargs on my fork
<headius> importmap seems to be working
<headius> ahh there's the NPE
<headius> app generation complets but the importmap section fails and causes a warning at the end
<enebo[m]> headius: you said merge it to master
<enebo[m]> so it is on there.
<headius> Oh ok
<headius> enebo: pushed that fix to master
<enebo[m]> ok
<enebo[m]> def insert_before(...)
<enebo[m]> @operations << -> middleware { middleware.insert_before(...) }
<enebo[m]> end
<enebo[m]> yay
<enebo[m]> So some more interesting digging for variables here
<enebo[m]> Although I don't really understand why this would even be broken...it should scope to *, **, & no problem
<headius> ick
<headius> so it needs to capture whatever ... args in the closure
<enebo[m]> I see a lot of this with -d enabled
<enebo[m]> It makes me think we are doing something on the wrong self/module
<enebo[m]> HAHAHA
<enebo[m]> # This suppresses the "method redefined" warning; the self-alias
<enebo[m]> # looks odd, but means we don't need to generate a unique name
<enebo[m]> alias_method method, method
<enebo[m]> ok I guess this is just some silly stuff that -d makes visible
<headius> oh, lovely
<enebo[m]> seems like a lot of redefinition though
<enebo[m]> each process seems to do this dozens of times
<headius> yeah can't say I've seen that in the past
<enebo[m]> although I guess maybe only twice per method which means it would be defined 3 times
<enebo[m]> so maybe something maybe not
<headius> the code for this attr definition has a bunch of warning-silencing code so it is also likely that silencing just doesn't work on JRuby
<headius> activesupport/core_ext/class/attribute.rb
<enebo[m]> headius: I am also running with -d and raw backtraces for debugging
<enebo[m]> This might not happen without that
<enebo[m]> %cl_1_4 = build_splat(*(-1:0:local=true))
<enebo[m]> Intriging
<enebo[m]> I am guessing -1 as depth is the error value
<headius> we warn in alias_method when replacing existing method, probably supposed to not do that
<enebo[m]> UnnamedRestArgNode:null line: 4
<headius> both of those warnings are from us in RubyModule.defineAlias
<enebo[m]> I maybe overloaded * the syntax with * the forward although I am not sure it should matter
<enebo[m]> yeah
<enebo[m]> The code I posted above shows alias method, method which I think somehow negates the warning
<headius> that's what I mean, it doens't
<headius> it actually triggers the warning
<headius> and we print two because it is PositionAware and we can show the old one
<enebo[m]> aha
<enebo[m]> well so MRI clearly does something different with this but I find it odf
<headius> looks like this should not warn when the new and old method are the same base method
<enebo[m]> So alias_method method, method immediately followed by define_method of the same method
<enebo[m]> really weird
<enebo[m]> what is the state of the method of that name between those two points
<enebo[m]> or what is alias of itself even mean
<headius> what did you run to get those redefine warnings?
<headius> I want to confirm this fix
<headius> I still see them for things like this but these are actually overwriting an existing def so warning might be proper:
<headius> $ rvm ruby-3.1 do ruby -w -e "class Foo; def foo; end; def bar; end; alias_method :foo, :bar; end"
<headius> -e:1: warning: method redefined; discarding old foo
<headius> -e:1: warning: previous definition of foo was here
<headius> so some of these suppressions can work with a patch to ignore same method, but this one does not appear to be suppressed
<headius> and is not same method
<enebo[m]> JRUBY_OPTS="--dev -Xbacktrace.style=raw -d" jruby -S rails new testv --skip-active-record
<headius> -w is enough
<enebo[m]> So -d in this case
<headius> it's a verbose warning
<enebo[m]> sure
<enebo[m]> I also recently made some fixes for warning categories but I don't think many of our warnings use a category
<headius> I will try that and see if some warnings are gone
<enebo[m]> which won't matter for -v or -w but it is another dimension
<headius> FWIW I have not been using --dev and results match yours
<headius> so that's good
<enebo[m]> yeah it appears our JIT issues is not dup'ing some things we are suppose to
<headius> hah I added -d and get all sorts of other internal error output and thought I broke something
<enebo[m]> So most likely we will not commonly see errors from that
<enebo[m]> hah yeah. it is fun to see all the .so etc trying to get loaded
<headius> those are the only warnings I have now
<headius> the first chunk there repeats a couple times for subprocesses
<headius> my patch should at least fix alias_method :foo, :foo
<headius> pushed that fix
<enebo[m]> nice
<headius> wow Ruby 3.1 prints a TON of warnings with -d
<headius> thousands and thousands... can this be right?
<enebo[m]> I think once they added categories they decided to add more
<enebo[m]> I also think it might be user-definable too?
<enebo[m]> LOL I made a mistake and should have made unnabledrestargnode "*" and not null BUT now instead of -1 it is -2
<headius> mostly I just see these from MRI:
<headius> /Users/headius/.rvm/gems/ruby-3.1.0/gems/zeitwerk-2.5.4/lib/zeitwerk/kernel.rb:24: warning: method redefined; discarding old require
<headius> /Users/headius/.rvm/gems/ruby-3.1.0/gems/bootsnap-1.11.1/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:8: warning: previous definition of require was here
<headius> I do not see the Time ones I showed above
<headius> hmmm
<headius> if (RTEST(ruby_verbose) &&
<headius> (old_def->alias_count == 0) &&
<headius> (!old_def->no_redef_warning) &&
<headius> type != VM_METHOD_TYPE_UNDEF &&
<headius> looks like they track if the old definition has been aliased and assume it's ok to overwrite?
<enebo[m]> ah yay
<enebo[m]> ok so they alias to itself which does nothing but mark it is as aliases without changing anything
<enebo[m]> it brings a tear to my eye that this is known semantics
<headius> yeah so this is some accounting they have added over the years to avoid warnings for these alias chains
<headius> we just don't have it
<headius> and in fact the aliasing to itself had some changes in 2022 that now specifically mark the method as no warning
<headius> most recent change: https://bugs.ruby-lang.org/issues/18516
<headius> the reference counting "alias_count" is from 2015
<headius> I will open a bug for this but it is low priority
<enebo[m]> lunch
<headius> good idea
<headius> enebo: we should get byteit101 blog post up too
<byteit101[m]> I'd like someone to ensure the code works as written on not-my-machine, but yes
<enebo[m]> yeah
<enebo[m]> headius: I just pushed fix for nested forwards. Rails console/generation should work now
<enebo[m]> It does for me anyways
<enebo[m]> headius: something I think you have more experience with me is GHA for JRuby projects. We need our activerecord-jdbc project moved from travis. Any chance you can do that?
<headius> yeah I can take a look
<headius> I will pull your change first and confirm it generates for me
<headius> enebo: why does it install ar-jdbc 50.0? Do we not have a 6.0 version?
<enebo[m]> It is because it never got yanked.
<enebo[m]> It has a poor specifier
<headius> oh I see
<enebo[m]> If we have a release for 7.0 then it goes away
<enebo[m]> I feel like it should get yanked but then I am leery of yanking gems
<headius> yeah it is drastic
<headius> confirmed app generates
<enebo[m]> Once we get a new CI running for ARJDBC then I think we can get out a release soon
<enebo[m]> HEH...13 safe tags can go away since safe is not a thing
<headius> thank god that is finally gone
<headius> this is no trivial CI workflow to swap over
<enebo[m]> THAT'S WHY I CALLED IN THE EXPERT
<headius> I'm honored
<enebo[m]> Actually I recall it is involved but I don't remember much past it loading versions of Rails
<enebo[m]> Unless I removed that
<enebo[m]> Pre-Rails 5 it would grab all supported versions of Rails to run tests against
<enebo[m]> but now we just only need the single version it supports
<enebo[m]> ah yeah...a lot of env stuff
<headius> 7.0 gems will have to run in jruby-head probably
<enebo[m]> yeah for sure
<headius> I will try to get something running today though
<enebo[m]> other branches will get adapter from the changes to master since each one has differences
<enebo[m]> but I am not too worried about that and I imagine once on is done it is just a little editing
<headius> took some fiddling to get the jobs to start at all but working on making them useful here: https://github.com/jruby/activerecord-jdbc-adapter/pull/1109
<enebo[m]> useful is good
<headius> bleh just getting mysql running is annoying
<headius> yeesh so many conditionals in this travis file
<headius> huzzah, I think I have the base DB setup working for mysql
<headius> I suppose I could have gotten this running quickly with SQLite
<enebo[m]> ConcurrencyError:
<enebo[m]> Detected invalid array contents due to unsynchronized modifications with concurrent users
<enebo[m]> # ./spec/compiler/general_spec.rb:394:in `block in <main>'
<enebo[m]> boo
<enebo[m]> I am down to a single failure and this would appear to be something not getting dup'd I think
<enebo[m]> def foo(a, (b, *, c), d, *e, f, (g, *h, i), j); [a,b,c,d,e,f,g,h,i,j]; end; p foo(1,[2,3,4],5,6,7,8,[9,10,11],12)
<headius> We can also get those errors if some array method walks off the end due to a bug
<enebo[m]> oh hmm if I print out the index then perhaps it is something like that
<enebo[m]> This particular case could be straining the processing of arguments since it likely does some exotic push/cats
<enebo[m]> I fixed a long-standing issue that the argument order should match the order the encode so instrs can depend on super() doing the correct thing
<enebo[m]> One reason it didn't start like that was sharing a base class with an additional field some instrs did not want to persist and load
<enebo[m]> # Java::JavaLang::ArrayIndexOutOfBoundsException:
<enebo[m]> # Index 8 out of bounds for length 8
<enebo[m]> So yeah this is walking off but it makes me wonder how this could be in JIT but not in interp
<enebo[m]> Perhaps one of the int fields are swapped around to a helper
<headius> Hmm yeah or jit is off by one on some arg walking
<enebo[m]> wot
<enebo[m]> realLength either changes or is wrong to begin with
<enebo[m]> I will figure that out now
<headius> ok that last push did database setup ok for mysql... I will work on this more in the evening