Skip to content

time_stubs.c: add fallback for Windows 7#13905

Merged
nojb merged 2 commits intoocaml:mainfrom
nojb:windows7
Mar 24, 2026
Merged

time_stubs.c: add fallback for Windows 7#13905
nojb merged 2 commits intoocaml:mainfrom
nojb:windows7

Conversation

@nojb
Copy link
Collaborator

@nojb nojb commented Mar 24, 2026

Adds a fallback for GetSystemTimePreciseAsFileTime for old versions of Windows (< 8).

Fixes #13871

@nojb nojb mentioned this pull request Mar 24, 2026
Closed
nojb added 2 commits March 24, 2026 13:43
@nojb
time_stubs.c: add fallback for Windows 7
2c4b7e7 
Signed-off-by: Nicolas Ojeda Bar <n.oje.bar@gmail.com>
@nojb
Changes
a4c3ffc 
Signed-off-by: Nicolas Ojeda Bar <n.oje.bar@gmail.com>
@Alizter Alizter self-requested a review March 24, 2026 12:45
Alizter
Alizter reviewed Mar 24, 2026
View reviewed changes
otherlibs/stdune/src/time_stubs.c
}
if (GetSystemTime == NULL) { /* < Windows 8 */
GetSystemTime = GetSystemTimeAsFileTime;
}
Copy link
Collaborator

@Alizter Alizter Mar 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an else and a printf here, however I am seeing that this is getting called quite a few times. Shouldn't this only be happening once? (On Win 11)

Copy link
Collaborator Author

@nojb nojb Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should be called only once per process. Could there be more than one Dune process active?

Copy link
Collaborator

@Alizter Alizter Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've solved the mystery. We have rules that generate the help pages so there were multiple dune processes happening when I did dune build dune.install. :-)

@nojb
Copy link
Collaborator Author

nojb commented Mar 24, 2026

CI failure is unrelated.

@nojb nojb merged commit 70c38ef into ocaml:main Mar 24, 2026
28 of 29 checks passed
@nojb nojb deleted the windows7 branch March 24, 2026 16:38
@Alizter Alizter mentioned this pull request Mar 24, 2026
Open
33 tasks
@Alizter
Copy link
Collaborator

Alizter commented Mar 24, 2026

We get this new warning on mingw:

time_stubs.c: In function 'dune_clock_gettime_realtime':
time_stubs.c:19:21: warning: assignment to 'void (*)(struct _FILETIME *)' from incompatible pointer type 'FARPROC' {aka 'long long int (*)()'} [-Wincompatible-pointer-types]
   19 |       GetSystemTime = GetProcAddress(h, "GetSystemTimePreciseAsFileTime");
      |                     ^

@Alizter Alizter mentioned this pull request Mar 25, 2026
Merged
@Alizter
Copy link
Collaborator

Alizter commented Mar 25, 2026

I've made #13914 to fix the warning.

Alizter added a commit that referenced this pull request Mar 25, 2026
@Alizter
fix(time): address warning on Windows in time_stubs.c (#13914)
d2120ac 
`GetProcAddress` returns `FARPROC` and therefore needs an explicit cast.
This fixes the warning introduced in
#13905.
@Alizter Alizter mentioned this pull request Mar 25, 2026
Open
@rgrinberg
Copy link
Member

rgrinberg commented Mar 26, 2026

Does OCaml 4.14 even support windows 7? If not, then this change is kind of pointless as it will be broken by the next release anyway.

@Alizter
Copy link
Collaborator

Alizter commented Mar 26, 2026

@rgrinberg I was able to test the patch in a Windows 7 VM so that's a yes.

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

Reviewers

@Alizter Alizter Alizter approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

dune 3.22.0 bootstrap fails on Windows < 8 — GetSystemTimePreciseAsFileTime not found in KERNEL32.dll

3 participants

@nojb @Alizter @rgrinberg