Arvados

20846-ruby3 @ f75a35d375ec8e9ae0160d2a847d96e96c2974e7

• `arvados-server install` chooses Ruby 3.2.2, and adds a couple of new dependency packages
• source:services/api and source:sdk/ruby have a few syntax updates to make tests pass

on debian:12 `arvados-server install` fails on Python trouble, pretty sure it's because ws4py is going stale.

on ubuntu:22.04 `arvados-server install` warns on python/ws4py trouble, but continues, and then fails on another Python thing while installing source:services/fuse:

go run ./cmd/arvados-package build -package-version=2.7.1rc1 -target-os ubuntu:22.04

...

Moving llfuse-1.5.0-py3.10-linux-x86_64.egg to /var/lib/arvados/lib/python/lib/python3.10/site-packages
Adding llfuse 1.5.0 to easy-install.pth file

Installed /var/lib/arvados/lib/python/lib/python3.10/site-packages/llfuse-1.5.0-py3.10-linux-x86_64.egg
error: setuptools 59.6.0 is installed but setuptools>=62.4.0 is required by {'python-daemon'}

The same setuptools problem quoted above also seems to happen on debian:11.

20846-ruby3 @ 067f70b50f31c37175a840e7e4e344983c468d10

Fixes setuptools issue, but there are still warnings about ws4py:

  File "build/bdist.linux-x86_64/egg/ws4py/async_websocket.py", line 87
    asyncio.async(closeit())
            ^^^^^
SyntaxError: invalid syntax

...and arvados-server install errors out while trying to install workbench1, which isn't surprising. Need to remove wb1 from lib/install.

20846-ruby3 @ 9ddc63bf01d4603bb373957ce9649da50e7ecd55

The reason my local virtualenv works and this branch doesn't is because I installed with pip and the branch installs with setup.py install, which is deprecated and takes a worse codepath. From the transcript:

+ for src in "/arvados/sdk/python" "/arvados/services/fuse" 
+ rsync -a --delete-after /arvados/sdk/python/ /var/lib/arvados/tmp/python/
+ cd /var/lib/arvados/tmp/python
+ python3 setup.py install
running install
/var/lib/arvados/lib/python/lib/python3.10/site-packages/setuptools/command/install.py:34: SetuptoolsDeprecationWarning: setup.py install is deprecated. Use build and pip and other standards-based tools.

Using the newly-reserved async keyword happens in ws4py.async_websocket. If you install with pip+wheel, this module never gets processed and so we sidestep the problem. If you try to actually run it, like setup.py install does, you run into trouble:

Python 3.11.2 (main, Mar 13 2023, 12:18:29) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import ws4py
>>> import ws4py.async_websocket
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/arvados/arvenv/lib/python3.11/site-packages/ws4py/async_websocket.py", line 87
    asyncio.async(closeit())
            ^^^^^
SyntaxError: invalid syntax

The branch should be updated so this install code follows a pattern similar to 20432a4533136a5ab9fa52c2e2ec2d90a855ecfb: run setup.py build if necessary (shouldn't hurt), then install with pip install PATH.

Fixes python usage ("setup.py build" + "pip3 install $path"), and an unrelated workbench2 build issue:

20846-ruby3 @ b24f2530ae41644a7eb9cfe28679182d76468737

Here's a branch with just the compatibility changes to our Ruby code that I needed to make tests pass in Ruby 3. I think it makes sense to merge this first, instead of simultaneously with the installer changes.

20846-ruby3-compat @ 88e18b7b9826b05e8485a6a99593ecda850969d7 -- developer-run-tests: #3879

Tom Clegg wrote in #note-15:

Here's a branch with just the compatibility changes to our Ruby code that I needed to make tests pass in Ruby 3. I think it makes sense to merge this first, instead of simultaneously with the installer changes.

This all looks fine. Just to make sure I'm following correctly, I believe this branch still maintains Ruby 2.7 compatibility, right? It's just making the keyword argument changes and gem updates necessary to also work on Ruby 3.0, yes?

Since I don't think Jenkins did it, can you please describe how you tested Ruby 3.0 compatibility? Did you run tests on a system where /var/lib/arvados/bin/ruby was Ruby 3.0? Did that run include the Ruby SDK tests?

Assuming I've got all that right, I think this is basically good to merge. One nit, in services/api/test/test_helper.rb in the last commit:

-    define_method method do |action, *args|
+    define_method method do |action, **args|

It seems like it would be good to rename args to kwargs here, since that's what it really is, and having a keyword argument hash named args is surprising. I know it makes a bigger diff but it seems like the right thing to do for readability long-term.

Thanks.

Brett Smith wrote in #note-16:

This all looks fine. Just to make sure I'm following correctly, I believe this branch still maintains Ruby 2.7 compatibility, right? It's just making the keyword argument changes and gem updates necessary to also work on Ruby 3.0, yes?

• this time I got an interesting new failure mode:
  

  + bin/rails db:environment:set
  rails aborted!
  LoadError: linked to incompatible 0W\226\322U - /home/tom/.gem/ruby/3.2.0/gems/ruby-prof-1.6.3/lib/ruby_prof.so
  ...
  + bin/rake db:setup
  rake aborted!
  LoadError: linked to incompatible `     nF\211U - /home/tom/.gem/ruby/3.2.0/gems/ruby-prof-1.6.3/lib/ruby_prof.so
  

  
  ...fixed (?) by removing $HOME/.gem/ruby/3.2.0/ and hitting install services/api again in run-tests.sh (maybe restarting run-tests.sh would have been enough, but I didn't think to try it first)
• some tests failed, fixed in 14c8fb3d5a

Since I don't think Jenkins did it, can you please describe how you tested Ruby 3.0 compatibility? Did you run tests on a system where /var/lib/arvados/bin/ruby was Ruby 3.0?

I ran arvados-server install -type test from 20846-ruby3 at b24f2530ae to get Ruby 3.2.2 installed at /var/lib/arvados/bin/ruby, then switched to this branch and ran tests using run-tests.sh interactive mode.

Did that run include the Ruby SDK tests?

Huh, apparently not. sdk/ruby-google-api-client, sdk/ruby, and sdk/cli now pass though, after some fixes in c51e59e03b.

It seems like it would be good to rename args to kwargs here

Indeed. Fixed.

20846-ruby3-compat @ c51e59e03bc721de2837db7958415766bd7b46c8 -- developer-run-tests: #3882

Tom Clegg wrote in #note-17:

20846-ruby3-compat @ c51e59e03bc721de2837db7958415766bd7b46c8 -- developer-run-tests: #3882

Following the same process as you (except I just did install deps before tests), I also installed Ruby 3 and I think this all passes for me too. (The failures I mentioned in the meeting are because I was still on the installer branch, my bad.)

I do get a couple of consistent deprecation errors. They don't need to be fixed in this branch, but if they don't get fixed in this ticket, they should get a follow-up story:

/home/brett/Curii/arvados/services/api/app/views/user_notifier/account_is_setup.text.erb:5: warning: Passing safe_level with the 2nd argument of ERB.new is deprecated. Do not use it, and specify other arguments as keyword arguments.
/home/brett/Curii/arvados/services/api/app/views/user_notifier/account_is_setup.text.erb:5: warning: Passing trim_mode with the 3rd argument of ERB.new is deprecated. Use keyword argument like ERB.new(str, trim_mode: ...) instead.

Either way, this looks good to me. Thanks.

Moved the ERB usage issue to #21169.

20846-ruby3 (install/boot/deps) branch rebased on main after 20846-ruby3-compat merge:

20846-ruby3 @ b77707a7b06d57145a7829458d476baf8573317e

arvados-server install now lets you specify an alternate Ruby version, so we will be able to use the latest arvados version to build two (or more) Jenkins images to confirm future changes pass tests on both Ruby versions.

Of course, debian12+ruby2.7.7 still doesn't work because of the openssl thing. But both should work on debian11.

Usage: arvados-server install [options] 
  -bundler-version version
        Bundler version to install (do not override in production mode) (default "2.2.19")
  -commit hash
        source commit hash to embed (blank means use 'git log' or all-zero placeholder)
  -eatmydata
        use eatmydata to speed up install
  -nodejs-version version
        Nodejs version to install (not applicable in production mode) (default "v12.22.12")
  -package-version string
        version string to embed in executable files (default "0.0.0")
  -ruby-version version
        Ruby version to install (do not override in production mode) (default "3.2.2")
  -singularity-version version
        Singularity version to install (do not override in production mode) (default "3.10.4")
  -source string
        source tree location (required for -type=package) (default "/arvados")
  -type type
        cluster type: development, test, production, or package (default "production")
  -version
        Write version information to stdout and exit 0

20846-ruby3 @ 9fa5faed898bf23fcea8f4e7946e540473e42e08 -- developer-run-tests: #3904

retry developer-run-tests-sdk-python-ruby: #3230

Tom Clegg wrote in #note-26:

20846-ruby3 @ 9fa5faed898bf23fcea8f4e7946e540473e42e08 -- developer-run-tests: #3904

I just want to make one background thing explicit to make sure we're on the same page: because this branch removes install and boot support for Workbench 1, it's not slated for a 2.7.x release, only 3.0. If you think that too, then we're good, there's no problem there.

My only other comment would be a UI thing about the version arguments. It feels like it would be frustrating that you're expected to prefix the NodeJS version with a v, and there's nothing in the help output or anything to tell you that. I understand the argument that we're just mirroring what NodeJS does, but I don't think anyone who isn't staring at this code deeply would think about whether the v is actually part of the version number, or just a marker. I think I would suggest that the validation for any version number we feel less sure about the format of be ^\d+(\.\d+)*$, and then the code should add the v prefix as necessary for the NodeJS version. Then everything's pretty consistent from a UI standpoint. That said, if you disagree, I wouldn't hold up a merge over it or anything.

Thanks.

Brett Smith wrote in #note-27:

I just want to make one background thing explicit to make sure we're on the same page: because this branch removes install and boot support for Workbench 1, it's not slated for a 2.7.x release, only 3.0. If you think that too, then we're good, there's no problem there.

Confirmed @ standup that Peter will be cherry-picking commits to make 2.7.1, so we're good to merge 3.0 branches to main.

the code should add the v prefix as necessary for the NodeJS version

Yeah, that makes more sense. Fixed.

20846-ubuntu2204 @ a77d65e098bc014d05c4c16cc14c5baa00afdd68

• adds ubuntu2204 target (from Lucas's 19213 branch)
• updates ruby to 3.2.2 so it doesn't fail on the openssl issue
• updates fpm (on all platforms) to a ruby3-compatible version
• adds debian12 target (since it's also unblocked by ruby 3)
• skips workbench1 packaging because that fails with ruby 3 (#20690 is about to remove it anyway)

• update Ruby install recipes (OS, RVM)
• recommend OS over RVM unless OS package is too old
• remove "install from source" option
• add ubuntu 22.04 and debian 12 to list of supported distributions

Should we avoid merging that last item until we merge the earlier changes and update jenkins to actually start publishing the packages?

Backed out the "add ubuntu 22.04 and debian 12 to list of supported distributions" commit and left it in a separate branch (20846-document-2204-support) to revisit after we have successfully auto-published the packages.

20846-ubuntu2204 @ b4ebaa2edbd67c695ea23f89e74c946b7f4eb221

Tom Clegg wrote in #note-31:

20846-ubuntu2204 @ b4ebaa2edbd67c695ea23f89e74c946b7f4eb221

• build/README notes that to add a new target, you must "Update the package download code near the bottom of `test_package_presence` in `run-library.sh` so it can download packages for the new distribution." Please add the new distros.
• Re the Python build changes in build/run-library.sh: The issue you noted in comments is going to keep coming up; see #20543. The Python standard library has included venv and ensurepip since 3.4, so the modern way to handle things would be to install Python; python3 -m venv VENVDIR; and then do everything inside the virtualenv, including installing setuptools (and upgrading pip if desired). I believe this should work on all our supported distributions.
  If you want to punt this, that's fine, but please make a follow-up story. Admittedly there's probably a lot of modernization we could do in our Python build stuff.
• In doc/_includes/_install_ruby_and_bundler.liquid, h4. Alma/CentOS/Red Hat 7: There isn't an Alma 7 either, so you can drop that along with Rocky.
• In the Makefiles, hardcoding make --jobs 8 seems mildly regressive, especially since we still count processors everywhere else. I don't feel too strongly about it, but what was your rationale for this change?

Thanks.

Brett Smith wrote in #note-32:

• build/README notes that to add a new target, you must "Update the package download code near the bottom of `test_package_presence` in `run-library.sh` so it can download packages for the new distribution." Please add the new distros.

Oops, added.

• Re the Python build changes in build/run-library.sh: The issue you noted in comments is going to keep coming up; see #20543. The Python standard library has included venv and ensurepip since 3.4, so the modern way to handle things would be to install Python; python3 -m venv VENVDIR; and then do everything inside the virtualenv, including installing setuptools (and upgrading pip if desired). I believe this should work on all our supported distributions.
  If you want to punt this, that's fine, but please make a follow-up story. Admittedly there's probably a lot of modernization we could do in our Python build stuff.

I was trying to minimize the "while we're here, let's do x" since adding 2204/12 already requires touching a lot of things. Added #21230 and put it on next sprint so we don't lose it.

• In doc/_includes/_install_ruby_and_bundler.liquid, h4. Alma/CentOS/Red Hat 7: There isn't an Alma 7 either, so you can drop that along with Rocky.

Fixed.

• In the Makefiles, hardcoding make --jobs 8 seems mildly regressive, especially since we still count processors everywhere else. I don't feel too strongly about it, but what was your rationale for this change?

Commit message in 981de3b943cb6da04145fb9e7f1ffcba171c9300:

    20846: Fix shell command in env var.

    With Ruby 3, something uses the MAKE var without the expected
    shell-eval, so the number-of-processors trick stopped working.

    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>

        make --jobs \$\(grep -c processor /proc/cpuinfo\) DESTDIR\=
        sitearchdir\=./.gem.20231120-15-fa6sx6 sitelibdir\=./.gem.20231120-15-fa6sx6
        clean
        make: invalid option -- 'c'
        Usage: make [options] [target] ...
        Options:
          -b, -m                      Ignored for compatibility.
          [...]

I just wasn't sure the speedup was worth the time to find the right way to spell it. Is "mildly regressive" a hint to follow up on this, or to not?

20846-ubuntu2204 @ 9329bd8bc74cdb4def31a0ced87a6013606db0a3

Tom Clegg wrote in #note-35:

I was trying to minimize the "while we're here, let's do x" since adding 2204/12 already requires touching a lot of things. Added #21230 and put it on next sprint so we don't lose it.

Of course, that's fine, thanks.

Commit message in 981de3b943cb6da04145fb9e7f1ffcba171c9300:

Thank you for spelling that out, love that commit message. I honestly don't care about whether we use 8 or the CPU count as a default, I think both have pros and cons, what bugs me more is the Dockerfile isn't internally consistent. But working around others' bugs is an understandable reason to do that.

What I'd really like to see is this decision handled at the Dockerfile level, I'll file my own ticket for that. This is good to merge, thanks.