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

[glog] fix not work on c++03 #25155

Merged
merged 6 commits into from
Jun 15, 2022
Merged

[glog] fix not work on c++03 #25155

merged 6 commits into from
Jun 15, 2022

Conversation

jiayuehua
Copy link
Contributor

@jiayuehua jiayuehua commented Jun 9, 2022

Describe the pull request
fix-windows-CXX11_ATOMIC.patch just "#include <atomic>" which not work on c++03.

  • What does your PR fix?

    Fixes #...

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    <all / linux, windows, ...>, <Yes/No>

  • Does your PR follow the maintainer guide?

    Your answer

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    <Yes / I am still working on this PR>

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

@Adela0814 Adela0814 self-assigned this Jun 9, 2022
@Adela0814 Adela0814 added the category:port-bug The issue is with a library, which is something the port should already support label Jun 9, 2022
@Adela0814
Copy link
Contributor

The previous change was to fix colmap build errors, I'm trying to install colmap with this PR. This may take some time......
@whuaegeanse could you please confirm this change.

@whuaegeanse
Copy link
Contributor

whuaegeanse commented Jun 9, 2022

The previous change was to fix colmap build errors, I'm trying to install colmap with this PR. This may take some time...... @whuaegeanse could you please confirm this change.

vcpkg only support vs 2015 or above which use c++ 14 by default. So we don't need to support c++03 on windows platform.
Including <windows.h> may make conflicts with other libs.
@Adela0814

@Adela0814
Copy link
Contributor

The previous change was to fix colmap build errors, I'm trying to install colmap with this PR. This may take some time...... @whuaegeanse could you please confirm this change.

vcpkg only support vs 2015 or above which use c++ 14 by default. So we don't need to support c++03 on windows platform. Including <windows.h> may make conflicts with other libs. @Adela0814

As far as I know, just removing the definition of NOGDI can fix the build error of colmap, I want to confirm if your patch has any other effect?

Comment on lines 14 to 16
+# ifndef NOMINMAX
+# define NOMINMAX
+# endif
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that you have removed NOGDI, is NOMINMAX necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember some ports will build fail if not define NOMINMAX. Maybe I can delete it and see if CI report error. The old error log #22135 (comment) I can't download.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know, just removing the definition of NOGDI can fix the build error of colmap, I want to confirm if your patch has any other effect?

Removing the definition of NOGDI can fix the build error of colmap, but CI of vcpkg will fail because of the macro ERROR defined in windows.h. It means that the macro ERROR defined in windows.h will get conflicts with other libs like rocksdb.

Including windows.h in glog is only to support C++03 on windows, and vcpkg only support C++14(C++11?) or above on windows. glog version less than 0.5 does not contain windows.h, and there are no errors when building colmap via vcpkg.

Beside, including windows.h in glog means many macros like MIN, MAX will be exported, and macros like NOMINMAX should be defined when using functions std::min/max.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jiayuehua @Adela0814

Removing the definition of NOGDI can fix the build error of colmap, but CI of vcpkg will fail because of the macro ERROR defined in windows.h. It means that the macro ERROR defined in windows.h will get conflicts with other libs like rocksdb.

Including windows.h in glog is only to support C++03 on windows, and vcpkg only support C++14(C++11?) or above on windows. glog version less than 0.5 does not contain windows.h, and there are no errors when building colmap via vcpkg.

Beside, including windows.h in glog means many macros like MIN, MAX will be exported, and macros like NOMINMAX should be defined when using functions std::min/max.

@dg0yt
Copy link
Contributor

dg0yt commented Jun 10, 2022

The problem is that public headers relying on changing WIN32_LEAN_AND_MEAN cause inclusion order problems for consumers.

#include <windows.h>
#include <evil_header.h>

will do something different than

#include <evil_header.h>
#include <windows.h>

The first form may break the evil header (by extra macros), the second form may break the consumer (by missing macros).

@whuaegeanse
Copy link
Contributor

whuaegeanse commented Jun 10, 2022

The problem is that public headers relying on changing WIN32_LEAN_AND_MEAN cause inclusion order problems for consumers.

#include <windows.h>
#include <evil_header.h>

will do something different than

#include <evil_header.h>
#include <windows.h>

The first form may break the evil header (by extra macros), the second form may break the consumer (by missing macros).

@dg0yt
How to fix this?

@dg0yt
Copy link
Contributor

dg0yt commented Jun 10, 2022

Can you help us to understand the original problem?
Neither from the initial post nor from the commit(s) it is obvious that this is based on a revert of fdeea72 (#24538), merged only three days ago. Plus a new approach to solve the original problem? The original patches are from #22135. By making this obvious, review would be easier.

So there are four variants:

  • pure glog 0.5 (no added patches):
    This versions added the problematic #include <Windows.h>. Upstream's decision.
    I guess this caused problems with some ports in vcpkg.
  • initial patching with [glog] update to v0.5 #22135:
    It adds dealing with WIN32_LEAN_AND_MEAN, NOGDI, and NOMINMAX, probably solving port issues. But potentially creating other port issues.
    But there are some unexpected changes from int LOG_OCCURRENCES to volatile unsigned LOG_OCCURRENCES, i.e. significantly changing upstream implementation for Windows.
  • patching in [glog] Glog nominmax #24538:
    This remove the mentioned changes, but forces HAVE_CXX11_ATOMIC on Windows, activating another set of macro definitions, i.e. significantly changing upstream implementation for Windows.
  • patching in this PR, [glog] fix not work on c++03 #25155:
    This restores the first changes.
    Does it add anything else?

Now there is another variant:

  • glog 0.6
    The changelog has some related entries:
    • cmake: export <atomic> availability
    • revert to signed int atomics

@jiayuehua
Copy link
Contributor Author

jiayuehua commented Jun 10, 2022

Can you help us to understand the original problem? Neither from the initial post nor from the commit(s) it is obvious that this is based on a revert of fdeea72 (#24538), merged only three days ago. Plus a new approach to solve the original problem? The original patches are from #22135. By making this obvious, review would be easier.

So there are four variants:

  • pure glog 0.5 (no added patches):
    This versions added the problematic #include <Windows.h>. Upstream's decision.
    I guess this caused problems with some ports in vcpkg.

Yes, no added patches make folly evpp etc build fail.

  • initial patching with [glog] update to v0.5 #22135:
    It adds dealing with WIN32_LEAN_AND_MEAN, NOGDI, and NOMINMAX, probably solving port issues.
    But potentially creating other port issues.

NOGDI make colmap build fail,

But there are some unexpected changes from int LOG_OCCURRENCES to volatile unsigned LOG_OCCURRENCES, i.e. significantly changing upstream implementation for Windows.

This is a change in upstream master branch, I just copy it to v0.5.

  • patching in [glog] Glog nominmax #24538:
    This remove the mentioned changes, but forces HAVE_CXX11_ATOMIC on Windows, activating another set of macro definitions, i.e. significantly changing upstream implementation for Windows.
  • patching in this PR, [glog] fix not work on c++03 #25155:
    This restores the first changes.
    Does it add anything else?

It undefines ERROR marco. Maybe all macro DEFINE or UNDEFINE like NOMINMAX should defined in other ports that rely on glog build failed? So that glog is very much the same behavior as upstream?

Now there is another variant:

  • glog 0.6
    The changelog has some related entries:

    • cmake: export <atomic> availability
    • revert to signed int atomics

@whuaegeanse
Copy link
Contributor

whuaegeanse commented Jun 10, 2022

  • patching in [glog] Glog nominmax #24538:
    This remove the mentioned changes, but forces HAVE_CXX11_ATOMIC on Windows, activating another set of macro definitions, i.e. significantly changing upstream implementation for Windows.

Including windows.h in glog 0.5 or 0.6 is only to support C++03 on Windows where atomic is not avaiable. For C++11 and above, using atomic are recommanded. The bug of using atomic has been mentioned and fixed google/glog#671 google/glog#667
vcpkg only support C++14(C++11?) or above on windows. Thus, it's safe to use atomic on Windows.
@jiayuehua @Adela0814 @dg0yt

PS:
Is including windows.h in glog/logging.h a good way? Any better solution for us to get rid of including windows.h in libs?

@jiayuehua
Copy link
Contributor Author

  • patching in [glog] Glog nominmax #24538:
    This remove the mentioned changes, but forces HAVE_CXX11_ATOMIC on Windows, activating another set of macro definitions, i.e. significantly changing upstream implementation for Windows.

Including windows.h in glog 0.5 or 0.6 is only to support C++03 on Windows where atomic not avaiable. For C++11 and above, using atomic are recommanded. The bug of using atomic has been mentioned and fixed google/glog#671 google/glog#667 vcpkg only support C++14(C++11?) or above on windows. Thus, it's safe to use atomic on Windows. @jiayuehua @Adela0814 @dg0yt

PS: Is including windows.h in glog/logging.h a good way? Any better solution for us to get rid of including windows.h in libs?

I add upstream cmake atomic check, thanks.

@jiayuehua jiayuehua marked this pull request as draft June 10, 2022 07:27
This reverts commit 17a80f2.
@jiayuehua
Copy link
Contributor Author

microsoft.vcpkg.pr (x64_windows_static) ##[error]vcpkg ci failed
At C:\a\1\s\scripts\azure-pipelines\test-modified-ports.ps1:172 char:5

  • throw "vcpkg ci failed"
    
  • ~~~~~~~~~~~~~~~~~~~~~~~
    
  • CategoryInfo : OperationStopped: (vcpkg ci failed:String) [], RuntimeException
  • FullyQualifiedErrorId : vcpkg ci failed
    ##[error]PowerShell exited with code '1'.

@Adela0814 , Would you please rerun microsoft.vcpkg.pr (x64_windows_static).

@dg0yt
Copy link
Contributor

dg0yt commented Jun 12, 2022

Would you please rerun microsoft.vcpkg.pr (x64_windows_static).

Wait for CI to finish, then do git commit --allow-empty -m "CI [skip actions]"; git push

@jiayuehua
Copy link
Contributor Author

Would you please rerun microsoft.vcpkg.pr (x64_windows_static).

Wait for CI to finish, then do git commit --allow-empty -m "CI [skip actions]"; git push

thanks.

@jiayuehua jiayuehua marked this pull request as ready for review June 13, 2022 03:29
@Adela0814 Adela0814 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jun 13, 2022
@jiayuehua
Copy link
Contributor Author

please merge to master , thanks

@vicroms vicroms merged commit 86ff75c into microsoft:master Jun 15, 2022
@jiayuehua jiayuehua deleted the glog-fix branch June 16, 2022 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants