Skip to content

add -DNOMINMAX to both WIN32 and WIN64#810

Merged
cynthiajoan merged 7 commits intomainfrom
bugfix/unity_build
Jan 12, 2022
Merged

add -DNOMINMAX to both WIN32 and WIN64#810
cynthiajoan merged 7 commits intomainfrom
bugfix/unity_build

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.

Reviewers

@dconeybe dconeybe dconeybe approved these changes

Assignees

@cynthiajoan cynthiajoan

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cynthiajoan @dconeybe