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

Refactor: Merge mwc into bmq #468

Merged
merged 1 commit into from
Oct 23, 2024
Merged

Conversation

hallfox
Copy link
Collaborator

@hallfox hallfox commented Oct 17, 2024

MWC or "MiddleWare Core" was a package group developed to support a myriad of applications at Bloomberg. It's been useful to share common middleware components between similar technologies, but doesn't make much sense to support as its own open source library. Moving forward we are merging it into the BMQ package group to better maintain it for the BlazingMQ project.

@hallfox hallfox requested a review from a team as a code owner October 17, 2024 20:34
@hallfox hallfox changed the title Merge mwc into bmq Refactor: Merge mwc into bmq Oct 17, 2024
@hallfox hallfox force-pushed the merge-mwc-bmq branch 2 times, most recently from 7af4108 to 761193a Compare October 17, 2024 20:54
@pniedzielski
Copy link
Collaborator

@hallfox some failing clang-format checks...probably (though not obviously, to me) related to your rename. Probably best to pay this debt down now rather than merge with failing CI.

@pniedzielski
Copy link
Collaborator

Tested this patch doxygen, thankfully nothing new was built. This should make it easier to build docs for former-MWC, though.

@pniedzielski
Copy link
Collaborator

Still see three references to mwc:

.github/workflows/build.yaml
166:          ctest -E mwcsys_executil.t --output-on-failure
.github/workflows/sanitizers/build_sanitizer.sh
286:CMD+="./run-env.sh ctest -E mwcsys_executil.t --output-on-failure"
CMakeLists.txt
117:  set(BMQ_TARGET_MWC_NEEDED            YES)

Copy link
Collaborator

@pniedzielski pniedzielski left a comment

Choose a reason for hiding this comment

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

I still see three references to mwc in the codebase (see comment), and one of them is for a unit test that is failing in CI.

  • Doxygen builds fine (does not run on former mwc components)
  • Builds fine
  • Unit test fails

Copy link

@bmq-oss-ci bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

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

Build 321 of commit d250020 has completed with FAILURE

Copy link
Collaborator

@pniedzielski pniedzielski left a comment

Choose a reason for hiding this comment

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

Is

CMakeLists.txt
117:  set(BMQ_TARGET_MWC_NEEDED            YES)

still needed? I see the variable assignment was removed at (former) line 140 in the PR.

Copy link

@bmq-oss-ci bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

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

Build 326 of commit 05a4ca4 has completed with FAILURE

MWC or "MiddleWare Core" was a package group developed to support
a myriad of applications at Bloomberg. It's been useful to share
common middleware components between similar technologies, but doesn't
make much sense to support as its own open source library. Moving
forward we are merging it into the BMQ package group to better maintain
it for the BlazingMQ project.

Signed-off-by: Taylor Foxhall <tfoxhall@bloomberg.net>
Copy link
Contributor

@chrisbeard chrisbeard left a comment

Choose a reason for hiding this comment

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

lgtm, maybe @pniedzielski can give a second sanity check here

Copy link

@bmq-oss-ci bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

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

Build 327 of commit ab17a4e has completed with FAILURE

@pniedzielski pniedzielski self-requested a review October 23, 2024 17:27
Copy link
Collaborator

@pniedzielski pniedzielski left a comment

Choose a reason for hiding this comment

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

let's merge 💯

@pniedzielski pniedzielski merged commit 7d0ecf3 into bloomberg:main Oct 23, 2024
40 checks passed
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.

8 participants