<rdubya>
it also looks like there may be some timezone related tests failing now for prepared statements in pg that I'm not sure if they are related to the work you were doing with the date/time stuff or not
akp has joined #jruby
akp has quit [Client Quit]
guilleiguaran has joined #jruby
<kares>
rdubya: well if its only needed for PG than it should not live in core
<rdubya>
kares: it means copying the whole method for 1 line if it doesn't. I don't think its needed for 5.1 support just 5.0
<kares>
we're stepping on each others work but I assumed that is normal :)
<kares>
esp. that cherry-pick is now giving me a hard time to track where I left with testing out time/date changes
<rdubya>
sorry about that
<kares>
rdubya: it might be fixed in latest 5.0
<kares>
not sure
<kares>
it used to be really weird - basically it did not report binds correctly in log
<kares>
and part of it was due that legacy piece - which is happening in core if I recall right
<rdubya>
yeah
<kares>
this piece is really confusing so I am not against duplicating -> copy-pasta in Postgres' adapter
<rdubya>
I'm not sure if it was PG specific or not but that's where i'm seeing the failure
<rdubya>
its a test in the rails test suite that gets ran for all adapters
<enebo>
maybe with some changes over time due to AR changing the test a little bit?
<rdubya>
as far as I can tell
<kares>
ah so passing the "legacy" way ... [nil, post.id]
<enebo>
rdubya: if so then we agreed that we can delete from our suite
<kares>
well I am not sure how important is that
<rdubya>
enebo: its part of the rails suite, not ours
<enebo>
oh so this is just asking about ar test...sorry :)
<kares>
the question I can not answer is whether its legit to exclude - I do not see binds being passed the 'legacy' way
<kares>
(I am no 50-stable)
<kares>
... except for that test
<enebo>
if AR has a test saying it should work with legacy arguments doesn't that imply someone in the wild might still be using it? Or did they just forget to remove it if it is internal only (is that a thing in Ruby?)?
<rdubya>
guessing it was for backwards compat with 4.2 that then got removed in 5.1?
<rdubya>
let me check to see if the test still exists in the 5.1 suite
<rdubya>
ah, yeah its still in the 5.1 test suite
<rdubya>
so it seems like we should probably still support it if they had a reason for keeping it around this long
<rdubya>
looks like it might be for people directly using Arel to manage binds
<rdubya>
@connection.select_all("SELECT * FROM posts WHERE id = #{Arel::Nodes::BindParam.new.to_sql}", nil, [[nil, post.id]])
<enebo>
rdubya: maybe for people who tune their stuff a lot?
<enebo>
but did it a while back
<rdubya>
really it could be anybody that is directly querying the db I guess and using bind params and hasn't updated them to the new type system
<enebo>
sounds like they are supporting it explicitly as API to me
<enebo>
whether many people do it perhaps is another question
<rdubya>
kares: maybe a good middle ground would be to override the default select_all and check for it in there?
<kares>
are we going to worry about this much?
<rdubya>
it looks like they only support it in select_all
<kares>
I do not see a similar 'hack' anywhere in core
<enebo>
rdubya: but you just fixed this right?
<rdubya>
i had fixed it previously
<kares>
I have reverted it due other stuff not working
<kares>
well part of it - as it seemed weird to re-map binds
<rdubya>
if we move it to only happen in select_all it would match AR and would mean it doesn't get processed as often
<kares>
can fix that if you feel like it matters much
<enebo>
seems like less of a discussion thing and more of an effort thing. If it is not much effort to fix then it is probably not worth talking about since AR supports it. If it is a big thing to fix then we should exclude it
<enebo>
Since AR tests it and they do not use it internally it clearly is a public API
<enebo>
but probably an uncommon one
<kares>
+1 I do not mind having it - removed it since it felt weird or at least on a weird spot
<kares>
haven't seen anything similar at exec_query in core
<enebo>
We do this in jruby core a lot as well. If something is oddball but takes a lot of effort we see if anyone reports it
<kares>
and if Rails has it but only for some method such as select_all
<kares>
well I would say f*ck it :)
<enebo>
I am not doing the work so I don't care :P
<kares>
... its just that I want to push a release and worry about real stuff
<enebo>
but if it is not hard to fix then it seems reasonable to fix it
<kares>
this Rails mantra is so frustrating ;(
<rdubya>
ok, I can get it into the select_all override fairly easily I think
<kares>
but I understand I might have messed up smt Rob already fixed
<enebo>
hey that merge was big and we are close to being totally in sync
<enebo>
once we move a little further forward we will be smooth sailing I think
<enebo>
but I think we need 50,51,52 release by end of month
<enebo>
and probably much sooner for 50/51
<rdubya>
that would be ideal
<enebo>
Ruby 2.5 support is very close to being green enough to be in more of a bake mode
<enebo>
which will be end of month
<enebo>
we still will be packing goodies in like bytelist_love and some perf things but those can almost be considered stretch goals for core
<enebo>
ARJDBC needs to be up to date enough for RubyKaigi :) At least based on my abstract so it will get some attention from me soon
<enebo>
How nice will it be to be caught up on compat on these two projects by beginning of summer?
<rdubya>
that would be sweet
<enebo>
rdubya: we really appreciate your help
<enebo>
kares: and you too.
<rdubya>
was considering trying to give a lightning talk on arjdbc at railsconf if there's something worth talking about
<enebo>
both of you have put a lot of energy into these projects
<enebo>
rdubya: Apr 17 would not be bad for 5{0,1}.x
<enebo>
that is about 20 days
<rdubya>
yeah
<kares>
good
<enebo>
not sure how far pg is but for me the biggest issue with arjdbc is not appropriate minitest-excludes which loads properly on travis
<enebo>
Ryan was nice enough to get a real release out so one thing improved
<rdubya>
kares: you got in the change to the gemfile to start using that right? will that fix the travis issue?
<enebo>
rdubya: so lightning talk showing it working and if we are happy with performance we can talk about that but I have no idea. I predict slower sans you ps work
<kares>
believe its working fine
<enebo>
kares: have you tried adding an exclude?
<kares>
yeah I see 8 skips as locally for mysql
<kares>
just checked
<kares>
so I think those are fine
<kares>
the current issue is why do I see much more failure on CI than locally
<rdubya>
yeah I wasn't sure what I would talk about if I did it, mostly just thought it would be a good way for me to get out of my comfort zone and potentially drum up some new interest in JRuby
<rdubya>
kares: yeah I was wondering that too, seemed like there were some odd failures that I don't see locally
<enebo>
kares: I love you bro!
<kares>
enebo: you shall know this in theory - at least for mysql seems like a lot of encoding
<enebo>
kares: I tried so many things
<kares>
being mixed on CI?
<kares>
pieces such as:
<enebo>
kares: but for 50.x I had virtually failures for either of these adapters before the merge
<enebo>
kares: wild. yeah something about db setup I bet
<kares>
9 failures, 5 errors, 8 skips on CI while locally 5 + 1
<kares>
yeah likely
<kares>
a collation not being UTF-8
<kares>
weird I thought it always had sensible defaults on Travis
<kares>
+ our own suite does not complain ;(
<enebo>
5+1 sounds much closer to what I recall (I sort of recall 4 but I think 1 was a bogus test for us from the mysql native test dir)
<enebo>
yeah
<kares>
it feels like yet some Rails magic to figure out ...
<enebo>
kares: yeah it could be. we still replicate their rake logic too right?
<kares>
do we? don't know
<enebo>
kares: I think you moved that logic into our rakefiles so we could incorporate excludes
<kares>
aah so this nails it: Illegal mix of collations (latin1_swedish_ci,IMPLICIT) and (utf8_general_ci,COERCIBLE)
<kares>
so its about figuring how did that swedish piece get there
<kares>
enebo: aah that well not exactly their logic - used my own logic pretty much ... just to get it rolling
<enebo>
kares: I think some tests will set those options in AR and our connection config piece is supposed to do the right thing
<enebo>
kares: ok
<kares>
and BTW on master it doesn't boot
<enebo>
kares: but the connection config is the most elaborate piece of code we have still
<kares>
was checking today
<kares>
and I do not feel like its due that mega merge
<enebo>
kares: master is still 51.x or tracking rails master?
<enebo>
I am forgetting too many details :)
<kares>
yep
<kares>
no worries there - am not sure it ever run Rails tests
<kares>
I found out - its a configuration leak somehow only happening on CI
<kares>
I see mysql's suite setting the swedish collation as you suggested
<lopex>
lolz String#split takes a block in 2.6
<lopex>
so many years
<kares>
so I will look into that I guess
<enebo>
oh wait master is still supposed to be 51.x
<rdubya>
enebo: master is still tracking 5.1
<enebo>
we should make a branch
<kares>
rdubya: mostly only looking on 50-stable but if you feel like date/time regression just write them out somehow in an issue or so
<enebo>
kares: origin/support-50-stable
<enebo>
is this temporary branch
<enebo>
maybe you did not make that
<rdubya>
kares: ok i'll try to look into them a bit and write up an issue
<kares>
was trying to get into those but its been a few days and I am constantly distracted elsewhere
<kares>
I just do not want those to be holding you back or blocking your work ...
<kares>
enebo: yeah I might have made that
<rdubya>
just noticed them
<rdubya>
they aren't blocking me at this point
<kares>
believe that was the PR
<enebo>
now that excludes work we should consider making individual issues and adding the issue number to each exclude
<rdubya>
enebo: +1
<enebo>
I think even for this mysql one it would be at most 15 but I will try and look into these soon
<enebo>
but if we can be green I think it will help and maybe even allow people to follow what we plan on fixing without having to run or read ci logs
<enebo>
kares: but if you do figure out that db encoding thing (guessing it is that) then that will only be 5/1
<kares>
for MySQL/SQLite yes
<kares>
but rdubya pbly noticed smt that I regressed in Postgres?
<enebo>
BasicsTest#test_respect_internal_encoding:ArgumentError: invalid byte sequence in EUC-JP
<enebo>
heh
<enebo>
ActiveRecord::AdapterTest#test_select_all_with_legacy_binds:ActiveRecord::StatementInvalid: NoMethodError: undefined method `type' for [nil, 1]:Array: SELECT * FROM posts WHERE id = ?
<enebo>
rdubya: ^ sqlite3
<kares>
yeah that is me as well
<kares>
the thing we talked about before :)
<enebo>
kares: but likely the same code
<rdubya>
yeah that will be fixed with the select_all fix that I'll put in
<kares>
rdubya: thanks
<rdubya>
hopefully anyways lol
<enebo>
kares: yeah I was just saying it is more than just pg as I guess rdubya mentioned before as well
<kares>
okay - have yet to see than
<kares>
as noticed I tried checking out master - how Rails 5.1 suites are doing
<kares>
on CI but they do not boot
<enebo>
well 20 between sqlite23 and mysql is not horrible to work through
<kares>
and I am primarily working of 50-stable
<enebo>
I know a few are bogus tests
<enebo>
although I guess there is ~10 skipped to re-examine
<kares>
that is why I asked for an issue
<kares>
most of those skips are legit
<kares>
wouldn't worry there
<enebo>
kares: ah ok good
<enebo>
our end game will probably be to exclude them in Rails via PR but excludes will give us a nice way of tracking this