8000 [Utils] Use gmtime_s with _WIN32 by Zannick · Pull Request #914 · Veil-Project/veil · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Utils] Use gmtime_s with _WIN32 #914

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
Merged
merged 1 commit into from
Apr 12, 2021
Merged

[Utils] Use gmtime_s with _WIN32 #914

merged 1 commit into from
Apr 12, 2021

Conversation

Zannick
Copy link
Collaborator
@Zannick Zannick commented Apr 1, 2021 •
edited
Loading

Problem

Following the build instructions for WSL with Ubuntu 20.04 resulted in this error:

utiltime.cpp: In function ‘std::string FormatISO8601DateTime(int64_t)’:
utiltime.cpp:85:5: error: ‘gmtime_r’ was not declared in this scope; did you mean ‘gmtime_s’?
   85 |     gmtime_r(&time_val, &ts);
      |     ^~~~~~~~
      |     gmtime_s

Manually changing gmtime_r to gmtime_s results in a further warning because the arguments are reversed, a sign that we're building with the Windows version that the code uses when _MSC_VER is set.

Root Cause

WSL with Ubuntu 20.04 is not building with msvc but with mingw.

Solution

Use the Windows version of this code when either _MSC_VER or __MINGW64__ is set.

(Note: I also needed to change the build command make HOST=x86_64-w64-mingw32 to CC_FOR_BUILD=x86_64-linux-gnu-gcc make HOST=x86_64-w64-mingw32 in order to build gmp, but I was not sure if I should update the docs.)

Bounty PR

None

Bounty Payment Address

sv1qqpswvjy7s9yrpcmrt3fu0kd8rutrdlq675ntyjxjzn09f965z9dutqpqgg85esvg8mhmyka5kq5vae0qnuw4428vs9d2gu4nz643jv5a72wkqqq73mnxr

Unit Testing Results

Tested on win64 mingw WSL/Ubuntu 20.04 by building successfully and running unittests successfully compared to master.

@Rock-N-Troll
Copy link

@codeofalltrades had mentioned to make this change to support windows as well. I was encountering this issue and lazily just defaulted to gmtime_s, but this is a much better approach.

@CaveSpectre11 CaveSpectre11 added Tag: Build System Tag: OS - Windows Specific to Windows operating system labels Apr 5, 2021
CaveSpectre11
CaveSpectre11 approved these changes Apr 5, 2021
View reviewed changes
Copy link
Collaborator
@CaveSpectre11 CaveSpectre11 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

be9d3ec

Ran a test with changing Github Actions to use Ubuntu 20. This does solve the Win64 issue with ubuntu 20; but the same problem exists for Win32; So it looks like it needs one more conditional on that # if.

See here:
https://github.com/CaveSpectre11/veil/runs/2274102054?check_suite_focus=true

@Zannick
[Utils] Use gmtime_s with _WIN32
aa7e26e 
This allows the project to build in WSL/Ubuntu 20.04 x64
and potentially other Windows versions.
@Zannick Zannick force-pushed the utiltime branch 2 times, most recently from e1b5e3c to aa7e26e Compare April 7, 2021 00:07
@Zannick
Copy link
Collaborator Author
Zannick commented Apr 7, 2021

According to touch /tmp/test.c; x86_64-w64-mingw32-gcc -E -dM /tmp/test.c | grep -i "mingw\|win" the compiler defines plenty of symbols like _WIN32, __WIN32, __WIN32__, __MINGW32__ and the 64 variants of those. Not sure which is really the most appropriate and I can't test Win32 specifically except by uploading here. I put _WIN32 to try to be more general (in case cygwin ever gets used?), but I can switch it to __MINGW32__.

@Zannick Zannick changed the title [Utils] Use gmtime_s with MINGW64 [Utils] Use gmtime_s with _WIN32 Apr 8, 2021
CaveSpectre11
CaveSpectre11 approved these changes Apr 10, 2021
View reviewed changes
Copy link
Collaborator
@CaveSpectre11 CaveSpectre11 left a comment

Choose a reason for hiding this comment

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

ACK aa7e26e

https://github.com/CaveSpectre11/veil/actions/runs/736392142 (Ubuntu 20 actions)

codeofalltrades
codeofalltrades approved these changes Apr 11, 2021
View reviewed changes
Copy link
Collaborator
@codeofalltrades codeofalltrades left a comment

Choose a reason for hiding this comment

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

ACK aa7e26e

@WetOne
Copy link
Collaborator
WetOne commented Apr 11, 2021

Ack aa7e26e

@codeofalltrades codeofalltrades merged commit 5852183 into Veil-Project:master Apr 12, 2021
@Zannick Zannick deleted the utiltime branch May 18, 2021 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CaveSpectre11 CaveSpectre11 CaveSpectre11 approved these changes

@codeofalltrades codeofalltrades codeofalltrades approved these changes

@WetOne WetOne Awaiting requested review from WetOne

Assignees

@Zannick Zannick

Labels
Code Review: Passed Tag: Build System Tag: OS - Windows Specific to Windows operating system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Zannick @Rock-N-Troll @WetOne @CaveSpectre11 @codeofalltrades
0