Skip to content

Fix unittests on windows(mingw) #12287

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
Feb 7, 2020
Merged

Fix unittests on windows(mingw) #12287

merged 1 commit into from
Feb 7, 2020

Conversation

maciejbocianski
Copy link
Contributor

Summary of changes

When on mingw (windows):

  • set equeue platform to posix
  • enable PRIx formatting globally
  • remove redundant Semaphore implementation

Impact of changes

Migration actions required

Documentation

Pull request type 

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@jamesbeyond
@michalpasztamobica - can you test on your side?

@ciarmcom ciarmcom requested review from michalpasztamobica and a team January 21, 2020 10:00
@ciarmcom
Copy link
Member

@maciejbocianski, thank you for your changes.
@michalpasztamobica @ARMmbed/mbed-os-maintainers please review.

michalpasztamobica
michalpasztamobica approved these changes Jan 21, 2020
View reviewed changes
Copy link
Contributor

@michalpasztamobica michalpasztamobica left a comment

Choose a reason for hiding this comment

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

After some trouble configuring my environment I managed to get unittests build and run on Windows command line and in Gitbash for the first time. No need to use WSL any more.
Great work. Thank you, @maciejbocianski .

bulislaw
bulislaw reviewed Jan 21, 2020
View reviewed changes
UNITTESTS/CMakeLists.txt
@@ -19,6 +19,16 @@ endmacro()

use_cxx14()


if (MINGW)
Copy link
Member

Choose a reason for hiding this comment

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

Is that the right way? I would rather make equeue work on Windows and Mac (for that matter), by enabling POSIX for these platforms in the platform abstraction layer of equeue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.
Updated

@maciejbocianski maciejbocianski force-pushed the fix_unittests_on_windows branch from 7fca1fc to adcd33b Compare January 21, 2020 17:30
@adbridge
Copy link
Contributor

@bulislaw are you happy with the update ?

@ladislas
Copy link
Contributor

ladislas commented Jan 28, 2020 •
edited
Loading

this PR conflicts with #12315 -- what is the preferred way to check for macOS?

@maciejbocianski
Copy link
Contributor Author

this PR conflicts with #12315 -- what is the preferred way to check for macOS?

According to https://stackoverflow.com/questions/142508/how-do-i-check-os-with-a-preprocessor-directive/42040445 __unix__ should work for macOS, if not __APPLE__/__MACH__ is next

@ladislas
Copy link
Contributor

__unix__ should work for macOS, if not __APPLE__/__MACH__ is next

on macOS Catalina (latest) it doesn't work with just __unix__ but does with __APPLE__, __MACH__ or both.

@ladislas
Copy link
Contributor

The website referenced in the SO answers says __APPLE__ && __MACH__

https://sourceforge.net/p/predef/wiki/OperatingSystems/

@maciejbocianski
Copy link
Contributor Author

maciejbocianski commented Jan 28, 2020 •
edited
Loading

@ladislas when #12315 will be merged I will rebase this one

@mergify
Copy link

mergify bot commented Jan 30, 2020

This PR cannot be merged due to conflicts. Please rebase to resolve them.

@kjbracey
Copy link
Contributor

#12315 has been merged

@maciejbocianski maciejbocianski force-pushed the fix_unittests_on_windows branch from adcd33b to b1af3d6 Compare January 31, 2020 05:55
@maciejbocianski
Copy link
Contributor Author

rebased

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 5, 2020

@maciejbocianski Can you improve the commit msg (before we run CI) ? To include details about the fix (they are in the description above).

@maciejbocianski
unittests: fix windows build
a2e5a34 
 - set equeue platform to posix for MINGW
 - enable PRIx formatting globally
 - remove redundant Semaphore implementation
@maciejbocianski maciejbocianski force-pushed the fix_unittests_on_windows branch from b1af3d6 to a2e5a34 Compare February 5, 2020 13:21
@maciejbocianski
Copy link
Contributor Author

done

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 5, 2020

CI started

@mergify mergify bot added the needs: work label Feb 5, 2020
@mergify mergify bot removed the needs: CI label Feb 5, 2020
@mbed-ci
Copy link

mbed-ci commented Feb 5, 2020

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 5, 2020

Tests restarted

@0xc0170 0xc0170 added release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 ready for merge and removed needs: work labels Feb 6, 2020
@0xc0170 0xc0170 merged commit e689153 into ARMmbed:master Feb 7, 2020
@mergify mergify bot removed the ready for merge label Feb 7, 2020
@maciejbocianski maciejbocianski mentioned this pull request Feb 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@michalpasztamobica michalpasztamobica michalpasztamobica approved these changes

@jamesbeyond jamesbeyond Awaiting requested review from jamesbeyond

@bulislaw bulislaw Awaiting requested review from bulislaw

Assignees
No one assigned
Labels
release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@maciejbocianski @ciarmcom @adbridge @ladislas @kjbracey @0xc0170 @mbed-ci @bulislaw @michalpasztamobica