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

Fix musl build (#1164) #1177

Merged
merged 1 commit into from Dec 15, 2023
Merged

Fix musl build (#1164) #1177

merged 1 commit into from Dec 15, 2023

Conversation

leso-kn
Copy link
Contributor

@leso-kn leso-kn commented Jul 10, 2023

As pointed out by @jefferyto the LFS64 interfaces were marked as deprecated in musl 1.2.4 (release notes, commit), thus go-sqlite3 currently fails to compile on musl.

This PR adjusts CLFAGS to let the C implementation use pread()/pwrite() instead of pread64()/pwrite64().

Note: This change does not affect existing glibc builds as suggested by the pread64 man page:

On Linux, the underlying system calls were renamed in kernel 2.6: pread() became pread64(), and pwrite() became pwrite64(). The system call numbers remained the same. The glibc pread() and pwrite() wrapper functions transparently deal with the change.

@leso-kn
Copy link
Contributor Author

leso-kn commented Jul 10, 2023

see #1164

@omerxx
Copy link

omerxx commented Jul 13, 2023

Honestly unsure how this isn't urgent to users out there but it saved me.
So for what it's worth I added this to my go.mod:

replace github.com/mattn/go-sqlite3 => github.com/leso-kn/go-sqlite3 v0.0.0-20230710125852-03158dc838ed

@ptrcnull
Copy link

hey @mattn could we get this merged? it's a really small non-breaking change that would fix all musl builds

@mattn
Copy link
Owner

mattn commented Sep 10, 2023

Can you add test for the musl build ?

@rittneje
Copy link
Collaborator

I had to do some digging here to check whether this change was safe, because on the surface it isn't. pread takes an off_t while pread64 takes an off64_t. So in a vacuum, this would break on systems where off_t is not 64 bits (overflow). (Note that the comment in the man page is referring to how the glibc wrapper invokes the underlying syscalls and is not relevant to this discussion.)

However, SQLite itself also sets _FILE_OFFSET_BITS to 64 (unless you set SQLITE_DISABLE_LFS), which forces off_t to be an alias for off64_t.

I do think there is a bug in the C library though, since it doesn't appear SQLite will attempt to constrain itself to 32 bits if LFS is disabled.

oskarirauta added a commit to oskarirauta/packages that referenced this pull request Oct 13, 2023
Bugfixes

 - Fixed a bug involving non-English locales of Windows where machine installs using user-mode networking were rejected due to erroneous version detection (openwrt#20209).
 - Fixed a regression in --env-file handling (openwrt#19565).
 - Fixed a bug where podman inspect would fail when stat'ing a device failed.

API
 - The network list compat API endpoint is now much faster (openwrt#20035).

Openwrt updates: added patch to allow building with musl-1.2.4
Patch source is from gentoo https://github.com/vimproved/gentoo/blob/c4c349f11a4352be1965726eadfe3a8bd8a6fa9c/app-containers/podman/files/podman-4.5.0-fix-build-with-musl-1.2.4.patch

Issue was discussed by @jefferyto at mattn/go-sqlite3#1177

Signed-off-by: Oskari Rauta <oskari.rauta@gmail.com>
1715173329 pushed a commit to openwrt/packages that referenced this pull request Oct 14, 2023
Bugfixes

 - Fixed a bug involving non-English locales of Windows where machine installs using user-mode networking were rejected due to erroneous version detection (#20209).
 - Fixed a regression in --env-file handling (#19565).
 - Fixed a bug where podman inspect would fail when stat'ing a device failed.

API
 - The network list compat API endpoint is now much faster (#20035).

Openwrt updates: added patch to allow building with musl-1.2.4
Patch source is from gentoo https://github.com/vimproved/gentoo/blob/c4c349f11a4352be1965726eadfe3a8bd8a6fa9c/app-containers/podman/files/podman-4.5.0-fix-build-with-musl-1.2.4.patch

Issue was discussed by @jefferyto at mattn/go-sqlite3#1177

Signed-off-by: Oskari Rauta <oskari.rauta@gmail.com>
oskarirauta added a commit to oskarirauta/packages that referenced this pull request Oct 14, 2023
Bugfixes

 - Fixed a bug involving non-English locales of Windows where machine installs using user-mode networking were rejected due to erroneous version detection (openwrt#20209).
 - Fixed a regression in --env-file handling (openwrt#19565).
 - Fixed a bug where podman inspect would fail when stat'ing a device failed.

API
 - The network list compat API endpoint is now much faster (openwrt#20035).

Openwrt updates: added patch to allow building with musl-1.2.4
Patch source is from gentoo https://github.com/vimproved/gentoo/blob/c4c349f11a4352be1965726eadfe3a8bd8a6fa9c/app-containers/podman/files/podman-4.5.0-fix-build-with-musl-1.2.4.patch

Issue was discussed by @jefferyto at mattn/go-sqlite3#1177

Signed-off-by: Oskari Rauta <oskari.rauta@gmail.com>
(cherry picked from commit e25d417)
oskarirauta added a commit to oskarirauta/packages that referenced this pull request Oct 14, 2023
Bugfixes

 - Fixed a bug involving non-English locales of Windows where machine installs using user-mode networking were rejected due to erroneous version detection (openwrt#20209).
 - Fixed a regression in --env-file handling (openwrt#19565).
 - Fixed a bug where podman inspect would fail when stat'ing a device failed.

API
 - The network list compat API endpoint is now much faster (openwrt#20035).

Openwrt updates: added patch to allow building with musl-1.2.4
Patch source is from gentoo https://github.com/vimproved/gentoo/blob/c4c349f11a4352be1965726eadfe3a8bd8a6fa9c/app-containers/podman/files/podman-4.5.0-fix-build-with-musl-1.2.4.patch

Issue was discussed by @jefferyto at mattn/go-sqlite3#1177

remarks:
removed musl-1.2.4 patch from commit, since that version of musl
is not available with openwrt-23.05

Signed-off-by: Oskari Rauta <oskari.rauta@gmail.com>
(cherry picked from commit e25d417)
1715173329 pushed a commit to openwrt/packages that referenced this pull request Oct 14, 2023
Bugfixes

 - Fixed a bug involving non-English locales of Windows where machine installs using user-mode networking were rejected due to erroneous version detection (#20209).
 - Fixed a regression in --env-file handling (#19565).
 - Fixed a bug where podman inspect would fail when stat'ing a device failed.

API
 - The network list compat API endpoint is now much faster (#20035).

Openwrt updates: added patch to allow building with musl-1.2.4
Patch source is from gentoo https://github.com/vimproved/gentoo/blob/c4c349f11a4352be1965726eadfe3a8bd8a6fa9c/app-containers/podman/files/podman-4.5.0-fix-build-with-musl-1.2.4.patch

Issue was discussed by @jefferyto at mattn/go-sqlite3#1177

remarks:
removed musl-1.2.4 patch from commit, since that version of musl
is not available with openwrt-23.05

Signed-off-by: Oskari Rauta <oskari.rauta@gmail.com>
(cherry picked from commit e25d417)
graysky2 pushed a commit to graysky2/packages that referenced this pull request Oct 23, 2023
Bugfixes

 - Fixed a bug involving non-English locales of Windows where machine installs using user-mode networking were rejected due to erroneous version detection (openwrt#20209).
 - Fixed a regression in --env-file handling (openwrt#19565).
 - Fixed a bug where podman inspect would fail when stat'ing a device failed.

API
 - The network list compat API endpoint is now much faster (openwrt#20035).

Openwrt updates: added patch to allow building with musl-1.2.4
Patch source is from gentoo https://github.com/vimproved/gentoo/blob/c4c349f11a4352be1965726eadfe3a8bd8a6fa9c/app-containers/podman/files/podman-4.5.0-fix-build-with-musl-1.2.4.patch

Issue was discussed by @jefferyto at mattn/go-sqlite3#1177

Signed-off-by: Oskari Rauta <oskari.rauta@gmail.com>
@ruzv
Copy link

ruzv commented Dec 8, 2023

any progress update on this?
musl 1.2.4 was released in May 1, 2023

@mattn
Copy link
Owner

mattn commented Dec 8, 2023

src/os_unix.c have this code. so I guess that this change should work well. @rittneje what do you think?

/* Use pread() and pwrite() if they are available */
#if defined(HAVE_PREAD64) && defined(HAVE_PWRITE64)
# undef USE_PREAD
# undef USE_PWRITE
# define USE_PREAD64 1
# define USE_PWRITE64 1
#elif defined(HAVE_PREAD) && defined(HAVE_PWRITE)
# undef USE_PREAD
# undef USE_PWRITE
# define USE_PREAD64 1
# define USE_PWRITE64 1
#endif

@leso-kn
Copy link
Contributor Author

leso-kn commented Dec 8, 2023

Thanks for the feedback @mattn, I've changed the patch to your version!

@mattn
Copy link
Owner

mattn commented Dec 9, 2023

Thanks for the feedback @mattn, I've changed the patch to your version!

Sorry, I made you confused. I just said that we can merge previous version of those changes.

@mattn
Copy link
Owner

mattn commented Dec 9, 2023

BTW, now I could reproduce this in https://github.com/mattn/nostr-relay/actions/runs/7151889396/job/19476647857

and confirmed with

#cgo linux,!android CFLAGS: -DHAVE_PREAD=1 -DHAVE_PWRITE=1 -DHAVE_PREAD64=1 -DHAVE_PWRITE64=1

https://github.com/mattn/nostr-relay/pull/2/files#diff-89278323f4af3d0fc58cd9680b5fa55549b914cb147c5f69b9cc4d06d26a780bR24

@mattn
Copy link
Owner

mattn commented Dec 9, 2023

Or we might be possible to remove this line. (not make sure)

sentriz added a commit to sentriz/gonic that referenced this pull request Dec 10, 2023
@james-d-elliott
Copy link

james-d-elliott commented Dec 11, 2023

For reference if anyone is having this issue in their CI/CD pipelines this affects the latest published golang alpine container image golang:1.21.5-alpine but doesn't affect golang:1.21.4-alpine. I suspect the 1.21.5 image was republished with the latest alpine/musl. It's also fairly likely that it'll affect the latest patch of each supported go version such as 1.20.12 as they likely republish these when the base image changes.

Edit: Looks like a golang:1.21.5-alpine3.18 tag exists. This alleviates the issue with container builds.

james-d-elliott added a commit to authelia/authelia that referenced this pull request Dec 11, 2023
Temporary fix for mattn/go-sqlite3#1177 which will be reverted in the near future as that's merged.

Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com>
james-d-elliott added a commit to authelia/authelia that referenced this pull request Dec 11, 2023
Temporary fix for mattn/go-sqlite3#1177 which will be reverted in the near future as that's merged.

Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com>
@mattn
Copy link
Owner

mattn commented Dec 13, 2023

Looks good to me.

I checked this change in mattn/nostr-relay#2

@leso-kn
Copy link
Contributor Author

leso-kn commented Dec 14, 2023

Rebased and switched back to the #ifdef-variant

@mattn
Copy link
Owner

mattn commented Dec 15, 2023

LGTM, Thank you.

@mattn mattn merged commit 00b02e0 into mattn:master Dec 15, 2023
11 checks passed
@ruzv
Copy link

ruzv commented Dec 15, 2023

Is there an ETA for the next release that would include this fix?

@james-d-elliott
Copy link

-7 hours ;) https://github.com/mattn/go-sqlite3/releases/tag/v1.14.19

emersion pushed a commit to emersion/soju that referenced this pull request Dec 21, 2023
go-sqlite3 does not build with musl 1.2.4+, which is packaged in some
distros (e.g. Alpine 3.19+). This update fixes it. More info:
mattn/go-sqlite3#1177
huiyifyj added a commit to Pengxn/go-xn that referenced this pull request Dec 22, 2023
The commit 031e41a is a temporary workaround to fix musl build error, and now to remove `CGO_CFLAGS="-D_LARGEFILE64_SOURCE"` for building docker image, because go-sqlite3 library was upgraded to `1.14.19` [^1] and the musl build was fixed in go-sqlite3 version `1.14.19` [^2].

This reverts commit 031e41a (refer PR #234).

---

[^1]: commit hash ad6673b
[^2]: mattn/go-sqlite3#1177
huiyifyj added a commit to Pengxn/go-xn that referenced this pull request Dec 23, 2023
Revert "Fix build docker with sqlite3 musl error (#234)" commit.

The commit 031e41a is a temporary workaround to fix musl build error, and now to remove `CGO_CFLAGS="-D_LARGEFILE64_SOURCE"` for building docker image, because go-sqlite3 library was upgraded to `1.14.19` [^1] and the musl build was fixed in go-sqlite3 version `1.14.19` [^2].

This reverts commit 031e41a (refer PR #234).

---

[^1]: commit hash ad6673b
[^2]: mattn/go-sqlite3#1177
FrimIdan added a commit to openclarity/kubeclarity that referenced this pull request Dec 26, 2023
FrimIdan added a commit to openclarity/kubeclarity that referenced this pull request Dec 26, 2023
github-merge-queue bot pushed a commit to openclarity/kubeclarity that referenced this pull request Dec 26, 2023
* build(deps): bump golang from 1.21.3-alpine to 1.21.5-alpine

Bumps golang from 1.21.3-alpine to 1.21.5-alpine.

---
updated-dependencies:
- dependency-name: golang
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* build(deps): update go-sqlite3 to resolve pread/pwrite buile issue following mattn/go-sqlite3#1177

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Idan Frimark <idanf@cisco.com>
jgiannuzzi pushed a commit to jgiannuzzi/go-sqlite3 that referenced this pull request Jan 22, 2024
jgiannuzzi pushed a commit to jgiannuzzi/go-sqlite3 that referenced this pull request Jan 22, 2024
use same workaround as authelia/authelia#6404
before mattn#1177 fixes the build
otherwise
jgiannuzzi pushed a commit to jgiannuzzi/go-sqlite3 that referenced this pull request Jan 22, 2024
jgiannuzzi pushed a commit to jgiannuzzi/go-sqlite3 that referenced this pull request Jan 22, 2024
use same workaround as authelia/authelia#6404
before mattn#1177 fixes the build
otherwise
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

9 participants