Skip to content

[fmt] Update to 7.0.2 + [spdlog] Update to 1.7.0 #12312

Merged
strega-nil merged 9 commits intomicrosoft:masterfrom
kevinlul:fmt-7.0.0
Aug 7, 2020
Merged

[fmt] Update to 7.0.2 + [spdlog] Update to 1.7.0 #12312
strega-nil merged 9 commits intomicrosoft:masterfrom
kevinlul:fmt-7.0.0

Conversation

@kevinlul
Copy link
Contributor

@kevinlul kevinlul commented Jul 7, 2020

  • What does your PR fix? Regular port update for {fmt}

  • Which triplets are supported/not supported? Have you updated the CI baseline?
    This causes a regression in spdlog because it uses the fmt implementation namespace, which was renamed from internal to detail in 7.0.0. I believe this can be resolved by defining the FMT_USE_INTERNAL macro according to the release notes, so I guess I should patch spdlog to add this define?

  • Does your PR follow the maintainer guide? Yes

@kevinlul

This comment has been minimized.

@kevinlul kevinlul changed the title [fmt] Update to 7.0.0 [fmt] Update to 7.0.1 Jul 7, 2020
@NancyLi1013
Copy link
Contributor

Hi @kevinlul
Thanks for your update.
The regressions for spdlog are due to this:

D:\buildtrees\spdlog\src\ac81386604-c2374a7a41\include\spdlog/details/fmt_helper.h(37): error C2039: 'internal': is not a member of 'fmt'
D:\installed\x86-windows\include\fmt/format.h(234): note: see declaration of 'fmt'
D:\buildtrees\spdlog\src\ac81386604-c2374a7a41\include\spdlog/pattern_formatter-inl.h(65): note: see reference to function template instantiation 'unsigned int spdlog::details::fmt_helper::count_digits<T>(T)' being compiled
        with
        [
            T=size_t
        ]

D:\buildtrees\spdlog\src\ac81386604-c2374a7a41\include\spdlog/details/fmt_helper.h(37): error C3083: 'internal': the symbol to the left of a '::' must be a type
D:\buildtrees\spdlog\src\ac81386604-c2374a7a41\include\spdlog/details/fmt_helper.h(37): error C2039: 'count_digits': is not a member of 'fmt'

Could you please look into these and try to fix them?

@NancyLi1013 NancyLi1013 added category:port-update The issue is with a library, which is requesting update new revision requires:author-response labels Jul 8, 2020
@kevinlul

This comment has been minimized.

@NancyLi1013
Copy link
Contributor

Hi @kevinlul
Thanks for looking into this so quickly.
For current status, we prefer to use a patch to fix the problem in spdlog. We can remove the patch in next spdlog release.
Since we don't know how long we need to wait for the next released version.

@AlexAUT
Copy link

AlexAUT commented Jul 10, 2020

spdlog 1.7.0 was just released with support for fmt 7.x https://github.com/gabime/spdlog/releases/tag/v1.7.0

@kevinlul kevinlul changed the title [fmt] Update to 7.0.1 [fmt] Update to 7.0.1 + [spdlog] Update to 1.7.0 Jul 11, 2020
@kevinlul

This comment has been minimized.

@kevinlul kevinlul force-pushed the fmt-7.0.0 branch 2 times, most recently from aa5778a to f9319a5 Compare July 11, 2020 06:08
@kevinlul

This comment has been minimized.

@anand-bala
Copy link
Contributor

@kevinlul For Windows, the definition should be /DFMT_USE_INTERNAL I think

@kevinlul

This comment has been minimized.

@NancyLi1013
Copy link
Contributor

The new regressions for quill:

D:\buildtrees\quill\src\76aaafff6d-cac280987f.clean\quill\include\quill/detail/BackendWorker.h(250): error C2220: the following warning is treated as an error
D:\buildtrees\quill\src\76aaafff6d-cac280987f.clean\quill\include\quill/detail/BackendWorker.h(250): warning C4530: C++ exception handler used, but unwind semantics are not enabled. Specify /EHsc

@kevinlul
Copy link
Contributor Author

quill patch should be done. Let's see what the CI thinks!

@kevinlul kevinlul requested a review from NancyLi1013 July 17, 2020 06:09
@NancyLi1013
Copy link
Contributor

If all checks have passed on CI. Could you please also test these features? Please make sure they can work on x86-windows, x64-windows and x64-windows-static, x64-linux triplets except for some special cases, such as the ports doesn't support some specific triplets.

Thanks.

@kevinlul
Copy link
Contributor Author

I think the build failures are from the current state of master.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kevinlul
Copy link
Contributor Author

kevinlul commented Jul 29, 2020

Need to bump to 7.0.2 in a bit

@kevinlul kevinlul changed the title [fmt] Update to 7.0.1 + [spdlog] Update to 1.7.0 [fmt] Update to 7.0.2 + [spdlog] Update to 1.7.0 Jul 30, 2020
@NancyLi1013 NancyLi1013 added requires:author-response and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Jul 30, 2020
@kevinlul kevinlul requested a review from NancyLi1013 July 31, 2020 03:04
@NancyLi1013
Copy link
Contributor

Have you test these features in this PR?

@kevinlul
Copy link
Contributor Author

Yes

@NancyLi1013 NancyLi1013 added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Aug 3, 2020
@strega-nil
Copy link
Contributor

Since quill has been updated to support fmt::detail, we don't need to worry about patching this anymore.

@kevinlul
Copy link
Contributor Author

kevinlul commented Aug 6, 2020

Guess that happens given enough time 😅 but I think with your last revert a bunch of whitespace got readded to a blank line in the quill portfile.

This was referenced Aug 6, 2020
@strega-nil
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@strega-nil
Copy link
Contributor

Yeah, I think that's fine :)

@strega-nil strega-nil merged commit 4f9117c into microsoft:master Aug 7, 2020
@strega-nil
Copy link
Contributor

strega-nil commented Aug 7, 2020

Thanks so much for your patience and work in pushing this forward @kevinlul !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-update The issue is with a library, which is requesting update new revision info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants