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.

Sign up for GitHub

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

Jump to bottom

Autotools gumbo #3221

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Autotools gumbo #3221

wants to merge 3 commits into from

Conversation

flavorjones
Copy link
Member

What problem is this PR intended to solve?

See conversation at #2718, this simplifies how we build libgumbo both during installation (extconf) and during development.

Primary author is @stevecheckoway

Have you included adequate test coverage?

Existing test coverage should be fine.

Does this change affect the behavior of either the C or the Java implementations?

No behavior changes.

@flavorjones flavorjones mentioned this pull request Jun 7, 2024
Closed
@stevecheckoway @flavorjones
Build libgumbo via autotools and rebuild on source changes
3c85081 
- Implement a minimal autotools build for libgumbo.
- Make `rake gumbo:test` use the same libgumbo.a used to build the
  nokogiri extension.
- Make changes to `gumbo-parser/src` trigger a rebuild of libgumbo.
@flavorjones flavorjones force-pushed the autotools-gumbo branch 3 times, most recently from 81c8a76 to 845d7cb Compare June 7, 2024 20:42
@flavorjones
Small changes to the libgumbo build branch
7a6613c 
- remove the `rebuild-libgumbo` target which is probably not needed
  once #3220 is merged
- avoid downloading googletest when just running `rake compile`
- update the Manifest check to ignore new gumbo-parser/ files
- simplify the 'host' variable in gumbo.rake, since we're only using
  it in development (and not cross-compiling)
- put back the libgumbo $libs, $LIBPATH, and include flags modifications
- make sure libgumbo is built static
@flavorjones flavorjones force-pushed the autotools-gumbo branch 3 times, most recently from 33b3156 to c47b232 Compare June 8, 2024 02:22
@flavorjones
Copy link
Member Author

I'm a little concerned about introducing autoconf to the development build process; I made some additional changes to help run it better in CI, but I think we probably also need to add a test job that installs the gem on a system without autoconf just to make sure we're packaging everything correctly.

@flavorjones
dev: make sure clobber removes all autoconf-generated files
6e6b368 
and make sure autoconf is installed in the basic ubuntu container in CI
@flavorjones
Copy link
Member Author

flavorjones commented Jun 8, 2024 •
edited
Loading

@stevecheckoway I wonder if we should split gumbo out into a separate repository? While it might make integration testing a little bit harder,

  1. it unblocks us from moving to autotools for libgumbo (this PR is going to need some more work to get through CI)
  2. it would make the Nokogiri build system more straightforward (parallel construction to libxml2 & libxslt)
  3. it would make the library available for third-party usage (and I think we might have the best-maintained fork at this point)

Just an idle thought on a Saturday morning.

Moka-Rezq

This comment was marked as spam.

Sign in to view

Moka-Rezq

This comment was marked as spam.

Sign in to view

@flavorjones
Copy link
Member Author

@stevecheckoway Bumping this in case you have any thoughts on what I suggested? I'm not sure how to unblock this one right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Moka-Rezq Moka-Rezq Moka-Rezq left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@flavorjones @Moka-Rezq @stevecheckoway