Skip to content

add -DNOMINMAX to both WIN32 and WIN64 #810

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
cynthiajoan merged 7 commits into from
Jan 12, 2022
Merged

add -DNOMINMAX to both WIN32 and WIN64 #810

cynthiajoan merged 7 commits into from
Jan 12, 2022

Conversation

@cynthiajoan
Copy link
Contributor

@cynthiajoan cynthiajoan commented Jan 10, 2022 •
edited
Loading

Description

Change WIN32 condition to MSVC, so -DNOMINMAX can be picked up by both win32 and win64
Add () outside std::numeric_limits::max, since the NOMINMAX definition didn't preventing max to be recognized as a macro.

Testing

Unity SDK build pass in windows GHA environment

Type of Change

Place an x the applicable box:

  • Bug fix. Add the issue # below if applicable.
  • New feature. A non-breaking change which adds functionality.
  • Other, such as a build process or documentation change.

Notes

  • Bug fixes and feature changes require an update to the Release Notes section of release_build_files/readme.md.
  • Read the contribution guidelines CONTRIBUTING.md.
  • Changes to the public API require an internal API review. If you'd like to help us make Firebase APIs better, please propose your change in a feature request so that we can discuss it together.
dconeybe
dconeybe requested changes Jan 11, 2022
View reviewed changes
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! I've left some drive-by comments. Do you know why this hasn't been a problem in the 64-bit Windows C++ SDK builds but is in the Unity builds?
@dconeybe
Copy link
Contributor

dconeybe commented Jan 11, 2022

Please ignore my comments. I did not pay close attention to the purpose of this PR.
@cynthiajoan cynthiajoan added the skip-release-notes Skip release notes check label Jan 11, 2022
@cynthiajoan cynthiajoan merged commit 9889ac9 into main Jan 12, 2022
@cynthiajoan cynthiajoan deleted the bugfix/unity_build branch January 12, 2022 00:48
@github-actions github-actions bot added the tests: in-progress This PR's integration tests are in progress. label Jan 12, 2022
@github-actions
Copy link

github-actions bot commented Jan 12, 2022 •
edited
Loading

❌  Integration test FAILED (but still ⏳  in progress)

Requested by @cynthiajoan on commit 9889ac9
Last updated: Tue Jan 11 18:28 PST 2022
View integration test log & download artifacts

Failures Configs
firestore [BUILD] [ERROR] [Windows] [openssl]
functions [BUILD] [ERROR] [MacOS] [openssl]
[BUILD] [ERROR] [Windows] [openssl]
installations [BUILD] [ERROR] [Android] [macos]
[BUILD] [ERROR] [MacOS] [openssl]
[BUILD] [ERROR] [Windows] [openssl]
messaging [BUILD] [ERROR] [Android] [macos]
[BUILD] [ERROR] [MacOS] [openssl]
[BUILD] [ERROR] [Windows] [openssl]
remote_config [BUILD] [ERROR] [Android] [macos]
[BUILD] [ERROR] [MacOS] [openssl]
[BUILD] [ERROR] [Windows] [openssl]
storage [BUILD] [ERROR] [Android] [macos, ubuntu]
[BUILD] [ERROR] [MacOS] [openssl]
[BUILD] [ERROR] [Windows] [openssl]

Add flaky tests to go/fpl-cpp-flake-tracker
@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Jan 12, 2022
DellaBitta added a commit that referenced this pull request Jan 21, 2022
@DellaBitta @jonsimantov
@sunmou99 @a-maurice @dconeybe @firebase-workflow-trigger @cynthiajoan
Merge main into GMA feature branch (#816)
fc18d67 
* Add a delay to the Analytics test app. (#788) * Add a delay when the Analytics test finishes. * In the test report, if test failed on all devices. Mark [All x test devices] (#784) * Disable -Wnullability-completeness on iOS and macOS. (#782) * Disable -Wnullability-completeness on iOS and macOS. * Remove NDK if the cache hit failed. * Update public documentation for C++ FCM SDK. (#789) As of Firebase C++ SDK 7.1.0, we started using JobIntentService for scheduling jobs. This change requires a few additional modifications to AndroidManifest.xml and a new way of starting a background service. These changes were done to the old repository, and need to be brought to GitHub. * Split mutex.h into .h and .cc files (#751) * future.h: Fix minor typo in the docs for FutureBase (#794) * Move mutex.h header to public internal (#792) * Delete app/src/mutex.h and adjust includes to app/src/include/firebase/internal/mutex.h (#795) * future.h: replace std::mutex with firebase::Mutex (#798) * Change setup-gcloud to use @v0 instead of @master as per their readme. (#802) * fix Log NoneType Error (#807) * Update Android dependencies - Thu Jan 06 2022 (#809) [Triggered](https://github.com/firebase/firebase-android-sdk/actions/runs/1665092765) by [firebase-android-sdk Jan 06 release](firebase/firebase-android-sdk@22ee484). ### Android - com.google.android.gms.play_services_base → 18.0.1 - com.google.firebase.firebase_analytics → 20.0.2 > Created by [Update Android and iOS dependencies workflow](https://github.com/firebase/firebase-cpp-sdk/actions/runs/1665095030). Co-authored-by: firebase-workflow-trigger-bot <firebase-workflow-trigger-bot@google.com> * add -DNOMINMAX to both WIN32 and WIN64 (#810) * add -DNOMINMAX to both WIN32 and WIN64 * test * test * test * test * remove helper print * format code Co-authored-by: Cynthia Jiang <cynthiajiang@google.com> * In the Test Summary Report, add "x/y" which means "x" out of "y" configs has errors. (#812) * mutex header include path * maven connectivity parameters (#818) CI scripts invoke gradle in a way to circumvent Azure-to-Maven connection pool timeout issues. Co-authored-by: Jon Simantov <jsimantov@google.com> Co-authored-by: Mou Sun <69009538+sunmou99@users.noreply.github.com> Co-authored-by: a-maurice <amaurice@google.com> Co-authored-by: Denver Coneybeare <dconeybe@google.com> Co-authored-by: firebase-workflow-trigger[bot] <80733318+firebase-workflow-trigger[bot]@users.noreply.github.com> Co-authored-by: firebase-workflow-trigger-bot <firebase-workflow-trigger-bot@google.com> Co-authored-by: Cynthia J <cynthiajoan@users.noreply.github.com> Co-authored-by: Cynthia Jiang <cynthiajiang@google.com>
@firebase firebase locked and limited conversation to collaborators Feb 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

skip-release-notes Skip release notes check tests: failed This PR's integration tests failed. tests: in-progress This PR's integration tests are in progress.

2 participants

@cynthiajoan @dconeybe