Skip to content

Port of spdlog to v1.11.0#17

Merged
Wmbat merged 8 commits intomainfrom
v1.11.0
Jun 7, 2023
Merged

Port of spdlog to v1.11.0#17
Wmbat merged 8 commits intomainfrom
v1.11.0

Conversation

@Wmbat
Copy link
Copy Markdown
Collaborator

@Wmbat Wmbat commented Jan 7, 2023

ci

@Wmbat Wmbat requested a review from Klaim January 7, 2023 16:34
@Wmbat Wmbat self-assigned this Jan 7, 2023
@Wmbat
Copy link
Copy Markdown
Collaborator Author

Wmbat commented Jan 7, 2023

There currently are errors in the CI builds at the Test phase

@Klaim
Copy link
Copy Markdown
Collaborator

Klaim commented Jan 7, 2023

Looks like formatter-bench.exe is waiting for specific input. Probably some testscript have to be updated following upstream changes.

@Wmbat
Copy link
Copy Markdown
Collaborator Author

Wmbat commented May 14, 2023

The CI is still failing: https://ci.cppget.org/@57c6f5f0-49fe-4d3d-a739-727c1d2bae15
I'm not sure why it's not accepting the input of "all" that i'm passing through the testscript i added. Do you any idea of what i did wrong ?

@Klaim

@Klaim
Copy link
Copy Markdown
Collaborator

Klaim commented May 14, 2023

Looks like it should work... I'll have to check locally.

@Klaim
Copy link
Copy Markdown
Collaborator

Klaim commented May 14, 2023

There seems to be issues with the symlinks on windows, Ill fix these first.

@Klaim
Copy link
Copy Markdown
Collaborator

Klaim commented May 14, 2023

I can reproduce the issue now, will attempt a fix.

@Klaim
Copy link
Copy Markdown
Collaborator

Klaim commented May 14, 2023

Ok the issue was that the testscript was not recognized as such. testscript files need to either be named testscript or have the .testscript extension. Fixing that seems to run the benchmark as expected.

@Klaim
Copy link
Copy Markdown
Collaborator

Klaim commented May 14, 2023

I just launched a ci request: https://ci.stage.build2.org/@04f3e9c2-4263-4e3f-8fa8-01aa5585f70b

@Klaim
Copy link
Copy Markdown
Collaborator

Klaim commented May 14, 2023

Welp looks like something else is wrong...

@Wmbat
Copy link
Copy Markdown
Collaborator Author

Wmbat commented May 14, 2023

we're getting configuration errors now:

error: unable to import target spdlog%lib{spdlog}

There is some problems when fetching the spdlog with git submodules

@Klaim
Copy link
Copy Markdown
Collaborator

Klaim commented May 14, 2023

Can you reproduce the issue locally? I can't.

I suspect there might be an issue with the test-packages relationships. welp it's not that

@Wmbat
Copy link
Copy Markdown
Collaborator Author

Wmbat commented May 14, 2023

I can't reproduce it either

@Klaim
Copy link
Copy Markdown
Collaborator

Klaim commented May 14, 2023

I tried to simplify the fix I did on the symlinks but that's not it: https://ci.stage.build2.org/@31d93a0e-bd43-450c-9bef-76de48d69fed

@Wmbat
Copy link
Copy Markdown
Collaborator Author

Wmbat commented Jun 5, 2023

I updated the package manifest file because i noticed that google-bench was no longer in "testing" but "stable".

CI: https://ci.cppget.org/@6cb41d52-e04c-403b-aa18-129ea371696b

@Wmbat
Copy link
Copy Markdown
Collaborator Author

Wmbat commented Jun 5, 2023

@Klaim The CI results of my last changes seem pretty good. What do you think of it ?
I see it having trouble with mingw and emcc but otherwise, this looks quite good to me

@Klaim
Copy link
Copy Markdown
Collaborator

Klaim commented Jun 5, 2023

Looks like the tests are randomly failing or not? Macos/Gcc seems to segfault, that might suggests some memory usage error so maybe making sure the upstream implementation is supposed to be tested correctly might be necessary.

@Klaim Klaim mentioned this pull request Jun 5, 2023
@Klaim
Copy link
Copy Markdown
Collaborator

Klaim commented Jun 5, 2023

IF the issues are from the original code, I guess we can publish the package or wait for a fixed version. If we publish it we can always revise or wait to publish the fixed version too.

Copy link
Copy Markdown
Member

@EmilRosenquist EmilRosenquist left a comment

Choose a reason for hiding this comment

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

I accidentally made the "update" without seeing this pull request (#18). But as @Klaim pointed out this already existed.

I tested this and it works. I think unless we want to patch this release with the upstream pull-request that fixed the include (gabime/spdlog#2545). This is good go!

@Wmbat
Copy link
Copy Markdown
Collaborator Author

Wmbat commented Jun 6, 2023

If we're all good to go with it. I'll merge the changes, add the tag and publish it to cppget. Doing another round check to make sure everyone is happy with it.

@Klaim
@EmilRosenquist

@Klaim
Copy link
Copy Markdown
Collaborator

Klaim commented Jun 7, 2023

LGTM

I think unless we want to patch this release with the upstream pull-request that fixed the include (gabime/spdlog#2545).

If we realize this is necessary we can always publish a revision for the same version.

@Wmbat Wmbat merged commit 29b0962 into main Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants