Skip to content

[loguru] Update to 2.1.0 and extend to generate proper binary on non-windows#8682

Merged
vicroms merged 5 commits intomicrosoft:masterfrom
PhoebeHui:dev/Phoebe/updateloguru
Jan 29, 2020
Merged

[loguru] Update to 2.1.0 and extend to generate proper binary on non-windows#8682
vicroms merged 5 commits intomicrosoft:masterfrom
PhoebeHui:dev/Phoebe/updateloguru

Conversation

@PhoebeHui
Copy link
Contributor

@PhoebeHui PhoebeHui commented Oct 21, 2019

Related to #8158

Loguru is not a header-only library any more, however, the source doesn't export to the lib on windows, so only install header on windows.

loguru has no feature that need to test locally.

@grdowns grdowns self-assigned this Nov 18, 2019
@JackBoosY
Copy link
Contributor

/azp run

@PhoebeHui PhoebeHui added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Dec 3, 2019
@dan-shaw dan-shaw removed the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Dec 10, 2019
@JackBoosY JackBoosY added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Dec 13, 2019
@JackBoosY JackBoosY self-requested a review December 13, 2019 06:08
@JackBoosY JackBoosY added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Jan 3, 2020
@JackBoosY
Copy link
Contributor

/azp run

2 similar comments
@PhoebeHui
Copy link
Contributor Author

/azp run

@JackBoosY
Copy link
Contributor

/azp run

@PhoebeHui PhoebeHui changed the title [loguru] extend to generate proper binary on non-windows [loguru] Update to 2.1.0 and extend to generate proper binary on non-windows Jan 20, 2020
@PhoebeHui
Copy link
Contributor Author

@vicroms, could you please priority to review and merge this PR?

@vicroms vicroms merged commit 5e329f0 into microsoft:master Jan 29, 2020
@BullyWiiPlaza
Copy link
Contributor

@PhoebeHui I have a problem with your loguru pull request. You're only pulling the header file (loguru.hpp) but the release also includes a source file (loguru.cpp). If you decide to use loguru with streams you will get a linker error: emilk/loguru#60 (comment)

I believe it would be necessary to also pull the implementation like I did in my pull request. If you have another way of making this work with your version, let me know and I might be happy.

@PhoebeHui
Copy link
Contributor Author

@BullyWiiPlaza, thanks for bringing this up, I will take a look.

@PhoebeHui PhoebeHui deleted the dev/Phoebe/updateloguru branch February 13, 2020 12:12
@PhoebeHui
Copy link
Contributor Author

@@BullyWiiPlaza, the PR has been merged, please get vcpkg source and try again.

@BullyWiiPlaza
Copy link
Contributor

@PhoebeHui For Windows it's working fine now but for Linux the *.cpp file is still not pulled. This wasn't a Windows specific issue. Please adapt the behavior for all operating systems to pull the *.cpp file.

@PhoebeHui
Copy link
Contributor Author

@BullyWiiPlaza, loguru is not header-only on linux now, it provides cmake targets on linux, you can use it, let me know if you encounter some issues about it.

find_package(loguru CONFIG REQUIRED)
target_link_libraries(main PRIVATE loguru)

@BullyWiiPlaza
Copy link
Contributor

@PhoebeHui
I still can't get it to compile since I can't successfully include the *.cpp file when it isn't found.

ubuntu:/bin/vcpkg$ sudo ./vcpkg install loguru
Computing installation plan...
The following packages are already installed:
    loguru[core]:x64-linux
Package loguru:x64-linux is already installed

Total elapsed time: 912.7 us

The package loguru:x64-linux provides CMake targets:

    find_package(loguru CONFIG REQUIRED)
    target_link_libraries(main PRIVATE loguru)

ubuntu:/bin/vcpkg$ cd installed/x64-linux/include/loguru
ubuntu:/bin/vcpkg/installed/x64-linux/include/loguru$ ls
loguru.hpp

Just to clarify, the C++ code I need to compile looks as follows:

#define LOGURU_WITH_STREAMS 1
#include <loguru/loguru.cpp>

Yes, including the *.cpp file is necessary once in the main.cpp, afterwards just including the *.hpp file works.

@PhoebeHui
Copy link
Contributor Author

@BullyWiiPlaza, sorry for my late reply, I will take a look at this issue again.

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

Labels

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.

7 participants