Skip to content
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

Load 'mini_portile2' only when needed. #381

Merged
merged 2 commits into from Mar 2, 2023

Conversation

voxik
Copy link
Contributor

@voxik voxik commented Feb 23, 2023

This enables installation using system SQLite3 without having mini_portile2 around:

gem install --force sqlite3 -- --enable-system-libraries

FTR, this is the process used by Fedora package, where we like to avoid maintaining additional dependencies unless really needed.

@voxik
Copy link
Contributor Author

voxik commented Feb 23, 2023

Hmm, is there one more place where the require should be needed?

  bundle exec ruby ./ext/sqlite3/extconf.rb --download-dependencies
  shell: /usr/bin/bash -e {0}
*** ./ext/sqlite3/extconf.rb failed ***
Could not create Makefile due to some reason, probably lack of necessary
libraries and/or headers.  Check the mkmf.log file for more details.  You may
need configuration options.

Provided configuration options:
	--with-opt-dir
	--without-opt-dir
	--with-opt-include
	--without-opt-include=${opt-dir}/include
	--with-opt-lib
	--without-opt-lib=${opt-dir}/lib
	--with-make-prog
	--without-make-prog
	--srcdir=./ext/sqlite3
	--curdir
	--ruby=/opt/hostedtoolcache/Ruby/3.1.3/x64/bin/$(RUBY_BASE_NAME)
	--help
	--download-dependencies
./ext/sqlite3/extconf.rb:135:in `minimal_recipe': uninitialized constant #<Class:Sqlite3::ExtConf>::MiniPortile (NameError)

        MiniPortile.new(libname, sqlite3_config[:version]).tap do |recipe|
        ^^^^^^^^^^^
	from ./ext/sqlite3/extconf.rb:176:in `download'
	from ./ext/sqlite3/extconf.rb:269:in `<main>'
Error: Process completed with exit code 1.

I should probably check this.

@flavorjones
Copy link
Member

@voxik Thanks for opening this PR! This seems like a reasonable request, I'll take a deeper look this weekend.

This enables installation using system SQLite3 without having
mini_portile2 around:

~~~
gem install --force sqlite3 -- --enable-system-libraries
~~~
@voxik
Copy link
Contributor Author

voxik commented Feb 28, 2023

I have moved the require into minimal_recipe. That seems to be the right place where the MiniPortile class is instantiated. This should make the test suite green.

@flavorjones
Copy link
Member

Kicked off CI again. Thanks!

@voxik
Copy link
Contributor Author

voxik commented Mar 1, 2023

Kicked off CI again. Thanks!

Thx, looks better this time 👍

@flavorjones
Copy link
Member

@voxik I'm in the process of adding a check for this to the CI suite. I'm having problems using the installed gem which I imagine aren't a problem for your Fedora build pipeline but wanted to just verify before proceeding ...

Here's what I think the test should look like:

    gem uninstall mini_portile2 --all --force
    gem install --local --force $gem -- --enable-system-libraries
    if gem list -i mini_portile2 ; then
      echo "should not have installed mini_portile, failing"
      exit 1
    fi
    ruby -r sqlite3 -e 'pp SQLite3::SQLITE_VERSION, SQLite3::SQLITE_LOADED_VERSION'

That last line, though, results in an exception like:

/home/flavorjones/.rbenv/versions/3.2.0/lib/ruby/3.2.0/rubygems/specification.rb:1452:in `rescue in block in activate_dependencies': Could not find 'mini_portile2' (~> 2.8.0) among 339 total gem(s) (Gem::MissingSpecError)
Checked in 'GEM_PATH=/home/flavorjones/.gem/ruby/3.2.0:/home/flavorjones/.rbenv/versions/3.2.0/lib/ruby/gems/3.2.0' at: /home/flavorjones/.rbenv/versions/3.2.0/lib/ruby/gems/3.2.0/specifications/sqlite3-1.6.1.gemspec, execute `gem env` for more information

I think the right thing to do in this test is to simply verify that the build can succeed, and not to worry about running the gem. Correct?

@voxik
Copy link
Contributor Author

voxik commented Mar 2, 2023

That last line, though, results in an exception like:

/home/flavorjones/.rbenv/versions/3.2.0/lib/ruby/3.2.0/rubygems/specification.rb:1452:in `rescue in block in activate_dependencies': Could not find 'mini_portile2' (~> 2.8.0) among 339 total gem(s) (Gem::MissingSpecError)
Checked in 'GEM_PATH=/home/flavorjones/.gem/ruby/3.2.0:/home/flavorjones/.rbenv/versions/3.2.0/lib/ruby/gems/3.2.0' at: /home/flavorjones/.rbenv/versions/3.2.0/lib/ruby/gems/3.2.0/specifications/sqlite3-1.6.1.gemspec, execute `gem env` for more information

Yes, that is not surprising. To avoid that issue, we drop the mini_portile2 runtime dependency during the build:

https://src.fedoraproject.org/rpms/rubygem-sqlite3/blob/cd3c7c24cc60ba92755fd0bdd2d98e9e5272382b/f/rubygem-sqlite3.spec#_42

I think this is acceptable modification, because neither the prebuilt gems have this dependency.

I think the right thing to do in this test is to simply verify that the build can succeed, and not to worry about running the gem. Correct?

Yes, right.

@flavorjones
Copy link
Member

Thanks. I'll push my test and re-run CI.

@flavorjones flavorjones merged commit 013f90f into sparklemotion:master Mar 2, 2023
@flavorjones
Copy link
Member

Merged! Will be in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants