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

github/workflow: remove the confusing comment #2227

Closed
wants to merge 1 commit into from

Conversation

tchaikov
Copy link
Contributor

@tchaikov tchaikov commented May 8, 2024

based on the feedback of reviewer,

the phrase "only build this combination when dpdk is enable" confusing

  • the phrase "in addition to the above" says the opposite, that we did
    build this combination without dpdk but now we'll build it again with
    dpdk.

@tchaikov tchaikov requested a review from nyh May 8, 2024 01:58
@nyh
Copy link
Contributor

nyh commented May 8, 2024

I'll merge this patch, but it gave me an opportunity to see the CI build in action. I see at the top of the page in the "checks" tab only 12 checks. It seems to list only the 12 regular combinations - and not the two extra checks you wanted to add for dpdk and modules. Are you sure those are running?

@tchaikov tchaikov force-pushed the github-workflow-comments branch 3 times, most recently from 06a6563 to 9a19de7 Compare May 8, 2024 11:15
before this change, we

* override the combination of <clang++,23,release>
  by adding settings to build with dpdk enabled,
* override the combination of <clang++,23,debug>
  by adding settings to build with C++20 moduels
  enabled.

but what we intended was to add two additional
builds.

so, in this change, we preserve the existing
combinations of <clang++,23,release> and
<clang++,23,debug> by includeing all the
combinations explicitly, and add the builds
with dpdk and C++20 modules. also the matrix
does not allows one to add extra settings without
overriding an existing combination.
see also https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs

also remove the confusing comment

based on the feedback of reviewer,

> the phrase "only build this combination when dpdk is enable" confusing
> - the phrase "in addition to the above" says the opposite, that we did
> build this combination without dpdk but now we'll build it again with
> dpdk.

Signed-off-by: Kefu Chai <[email protected]>
Copy link
Contributor

@nyh nyh left a comment

Choose a reason for hiding this comment

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

Having to unroll these loops is ugly, but better ugly than wrong and confusing :-)

@nyh nyh closed this in d6d4784 May 8, 2024
@tchaikov tchaikov deleted the github-workflow-comments branch May 8, 2024 11:40
@tchaikov
Copy link
Contributor Author

tchaikov commented May 9, 2024

there are still 12 builds. i need to find anther way to fix the matrix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants