Introduce MSVC CI support by wusatosi · Pull Request #80 · bemanproject/exemplar

wusatosi · 2024-11-19T20:53:23Z

This PR adds MSVC CI support.

Could be improved after #76 is merged.

Conflict with unknown state PR #66 .

Closes #24 .

camio

A couple nits on platform-independent syntax, but otherwise looks good.

camio · 2024-12-04T19:11:11Z

+            # MSVC has limited support for sanitizers, so we use include here for MSVC as an exception.
+            # This should be simplified after beman-project/exemplar#76 is merged.


Suggested change

# MSVC has limited support for sanitizers, so we use include here for MSVC as an exception.

# This should be simplified after beman-project/exemplar#76 is merged.

It isn't clear what is meant by "use include here". Also, it is unclear if #76 in its current form will be merged.

yeah sorry I should check my comment

camio · 2024-12-04T19:17:47Z

Sign in to view

          cmake --build build --config Release --target all_verify_interface_header_sets
          cmake --install build --config Release --prefix /opt/beman.exemplar
-          find /opt/beman.exemplar -type f
+          tree /opt/beman.exemplar


Maybe something like this?

Suggested change

tree /opt/beman.exemplar

cd /opt/beman.exemplar

tree

cd -

that's a great idea!

I don't think this works?

Agh, it's a quirk with powershell tree.

See: https://superuser.com/questions/1270040/windows-10-powershell-tree-command-how-to-show-files

switch out to ls -R, and it is working. It's just very ugly...

You can use the same trick I suggested in this comment to use the /F flag on windows only

camio · 2024-12-04T19:29:04Z

Sign in to view

+          ${{ matrix.platform.cpp }} --version
+          ${{ matrix.platform.c }} --version


Note that --version isn't a cl.exe flag. Maybe something like this? See this reference page.

Suggested change

${{ matrix.platform.cpp }} --version

${{ matrix.platform.c }} --version

${{ matrix.platform.cpp }} ${{ matrix.platform.cpp == 'cl' && '' || '--version' }}

${{ matrix.platform.c }} ${{ matrix.platform.cpp == 'cl' && '' || '--version' }}

Co-authored-by: David Sankel <[email protected]>

This reverts commit 01bf599.

neatudarius · 2024-12-05T11:16:33Z

          cmake --build build --config Release --target all_verify_interface_header_sets
          cmake --install build --config Release --prefix /opt/beman.exemplar
-          find /opt/beman.exemplar -type f
+          ls -R /opt/beman.exemplar


why find is not good?

do we just prefer a recursive display? If so, why not using tree?

Because we need to execute this on windows runner for MSVC, where tree doesn't work with the /opt/.... Directory.

Should I include a comment here documenting this?

OK, got it, right. I would add a command like name: Build Release # Portable commands only. thanks!

neatudarius · 2024-12-05T11:17:09Z

-        compiler:
-          - cpp: g++
+        platform:
+          - description: "ubuntu gcc"


Suggested change

- description: "ubuntu gcc"

- description: "Ubuntu GCC"

if it's in description?

camio

The # Portable commands only comments clutter up the code in my opinion, but I don't want to hold up merging for that :-D

wusatosi · 2024-12-06T22:04:51Z

The # Portable commands only comments clutter up the code in my opinion, but I don't want to hold up merging for that :-D

Yeah I do agree it clutters up the code...

Edit: I will wait for @neatudarius to confirm review before merging. Maybe there's a better way to comment this?

steve-downey · 2024-12-06T22:22:49Z

+          # Portable commands only
          cmake --build build --config Debug --verbose
          cmake --build build --config Debug --target all_verify_interface_header_sets
          cmake --install build --config Debug --prefix /opt/beman.exemplar


This install isn't going to do anything because it's installing to the same location as the Release install. Unless that's the intent?

Intention here is to test the install command.

It ends up skipping installation because it's already installed, which is a sort of test. Not critical for header only, but if there's any generated config or binaries, they shouldn't clobber each other.

steve-downey · 2024-12-07T15:58:16Z

Perhaps an explanatory general comment, once, reminding us to use the portable subset of commands.
On the other hand, if it breaks in CI, a PR that doesn't use the portable subset will fail?

wusatosi · 2024-12-10T19:02:53Z

Perhaps an explanatory general comment, once, reminding us to use the portable subset of commands. On the other hand, if it breaks in CI, a PR that doesn't use the portable subset will fail?

Ops I didn't see this before merging. I don't think this is fine. We can move this out in a later PR.

wusatosi added 6 commits November 14, 2024 12:53

init windows CI

2bf94fa

update config

37fe7ce

temp find -> tree

625f140

ugly workaround

1bd9055

add debug info for msvc asan

8c4c8a4

cleanup

47cc829

wusatosi requested review from bretbrownjr, camio, dietmarkuehl, neatudarius and steve-downey as code owners November 19, 2024 20:53

wusatosi commented Nov 19, 2024

View reviewed changes

Comment thread .github/workflows/ci_tests.yml Outdated

This was referenced Nov 19, 2024

Introduce CMake toolchain #76

Merged

Feature/support macos builds #66

Closed

camio reviewed Dec 4, 2024

View reviewed changes

wusatosi and others added 7 commits December 4, 2024 15:23

Update .github/workflows/ci_tests.yml

45e1005

Co-authored-by: David Sankel <[email protected]>

Update .github/workflows/ci_tests.yml

01bf599

Co-authored-by: David Sankel <[email protected]>

Update .github/workflows/ci_tests.yml

ac89671

Co-authored-by: David Sankel <[email protected]>

Revert no --version on cl

2230b3e

This reverts commit 01bf599.

use temporary directory for install

7f774ab

Use ls -R instead of tree for compatibility

e8f6417

fix print software version for cl

2286fa6

wusatosi force-pushed the windows-ci branch from 643a39e to 2286fa6 Compare December 5, 2024 00:40

wusatosi requested a review from camio December 5, 2024 00:43

wusatosi added 2 commits December 4, 2024 19:57

update configuration testing

84a7177

revert back to use /opt/beman.exemplar

1a3c911

neatudarius reviewed Dec 5, 2024

View reviewed changes

wusatosi added 2 commits December 5, 2024 17:06

add comment

fe6fc45

capitalize description

bdb06fc

wusatosi requested a review from neatudarius December 5, 2024 22:11

camio approved these changes Dec 6, 2024

View reviewed changes

steve-downey reviewed Dec 6, 2024

View reviewed changes

wusatosi merged commit 589d0f8 into bemanproject:main Dec 10, 2024

		# MSVC has limited support for sanitizers, so we use include here for MSVC as an exception.
		# This should be simplified after beman-project/exemplar#76 is merged.

		${{ matrix.platform.cpp }} --version
		${{ matrix.platform.c }} --version

Conversation

wusatosi commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

camio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wusatosi Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neatudarius Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

camio left a comment

Choose a reason for hiding this comment

Uh oh!

wusatosi commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steve-downey commented Dec 7, 2024

Uh oh!

wusatosi commented Dec 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wusatosi commented Nov 19, 2024 •

edited

Loading

wusatosi Dec 4, 2024 •

edited

Loading

neatudarius Dec 5, 2024 •

edited

Loading

wusatosi commented Dec 6, 2024 •

edited

Loading