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

Use the autoconf- or system-provided off_t rather than redetecting. #8843

Merged
merged 1 commit into from Jul 31, 2019

Conversation

@stedolan
Copy link
Contributor

@stedolan stedolan commented Jul 29, 2019

Using a 64-bit file offset on a 32-bit system is broken in 4.08, because the use of off_t is guarded by HAS_OFF_T, which is not defined by the configure script. (It's mentioned in configure.ac, but there's no corresponding template line in s.h.in).

As suggested by @rwmjones in #8841, this just uses off_t unconditionally. (Except on Windows, where off_t is weird and broken).

@stedolan stedolan force-pushed the use-off-t-directly branch from 1b261fb to 90ad648 Jul 29, 2019
@rwmjones
Copy link
Contributor

@rwmjones rwmjones commented Jul 29, 2019

Patch looks sensible to me.

@gasche
Copy link
Member

@gasche gasche commented Jul 29, 2019

cc @Octachron: would that be a candidate for 4.08.1?

Copy link
Contributor

@xavierleroy xavierleroy left a comment

Looks very good to me. The type off_t is part of POSIX 1-2008, perhaps earlier, so it should be available everywhere except under Windows.

I'm in favor of backporting this fix to 4.09, perhaps even to 4.08.1.

@Octachron
Copy link
Member

@Octachron Octachron commented Jul 29, 2019

Then let's backport this fix in 4.08.1 since a rc3 is already planned.

@rwmjones
Copy link
Contributor

@rwmjones rwmjones commented Jul 30, 2019

I'm currently rebuilding the Fedora packages with this patch to see if there are any problems.

@stedolan stedolan force-pushed the use-off-t-directly branch from 90ad648 to 8b85964 Jul 31, 2019
@xavierleroy
Copy link
Contributor

@xavierleroy xavierleroy commented Jul 31, 2019

off_t was already there in POSIX.1 back in 1990. I'll merge in trunk, check CI, then cherry-pick to 4.09 and 4.08.1.

@xavierleroy xavierleroy merged commit 5e4b55d into ocaml:trunk Jul 31, 2019
1 of 2 checks passed
xavierleroy added a commit that referenced this issue Jul 31, 2019
xavierleroy added a commit that referenced this issue Jul 31, 2019
@xavierleroy
Copy link
Contributor

@xavierleroy xavierleroy commented Jul 31, 2019

Cherry-picked to 4.08 branch (25b9020) and to 4.09 branch (18793a6). Hope I didn't mess it up!

@Octachron
Copy link
Member

@Octachron Octachron commented Jul 31, 2019

It looks fine, thanks!

@rwmjones
Copy link
Contributor

@rwmjones rwmjones commented Jul 31, 2019

I'm (still) testing the Fedora ocaml packages. It's going much slower than I expected because our build system is having problems.

@Octachron
Copy link
Member

@Octachron Octachron commented Jul 31, 2019

Ok, I will wait a bit for the build result before releasing the rc3.

@rwmjones
Copy link
Contributor

@rwmjones rwmjones commented Jul 31, 2019

I can confirm that this fixes the original bug (in supermin).

@Octachron
Copy link
Member

@Octachron Octachron commented Jul 31, 2019

Thanks for the confirmation!

hannesm added a commit to hannesm/ocaml-freestanding that referenced this issue Aug 3, 2019
… by posix)

since ocaml/ocaml#8843 (>= 4.08.1+rc3) off_t in sys/types.h is required
lpw25 added a commit to janestreet/ocaml that referenced this issue Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants