You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@serf.apache.org by "rinrab (via GitHub)" <gi...@apache.org> on 2025/06/07 12:11:32 UTC
[PR] cmake imporovements [serf]
rinrab opened a new pull request, #6:
URL: https://github.com/apache/serf/pull/6
(all commits are separate changes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@serf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] cmake imporovements [serf]
Posted by Timofei Zhakov <ti...@chemodax.net>.
On Sun, Jun 8, 2025 at 12:52 PM Branko Čibej <br...@apache.org> wrote:
>
> On 8. 6. 25 12:32, Timofei Zhakov wrote:
> > On Sat, Jun 7, 2025 at 2:52 PM Branko Čibej<br...@apache.org> wrote:
> >> On 7. 6. 25 14:11, rinrab (via GitHub) wrote:
> >>> rinrab opened a new pull request, #6:
> >>> URL:https://github.com/apache/serf/pull/6
> >>>
> >>> (all commits are separate changes)
> >> We don't have to change the generator to achieve this; MSBuild is
> >> already multi-config. The reason why I haven't made the equivalent
> >> change yet is that, if debug build or test fail, the release build will
> >> never happen. I'm still thinking about the ramifications of that, but I
> >> for sure do not want to split the different config builds into two
> >> separate jobs. Or maybe I do, I'll have to figure that out.
> >>
> >> -- Brane
> > Yes, this is not mandatory to change the generator. But as I already
> > mentioned (I think in the "GitHub actions notifications" thread),
> > Ninja generator is much better for automation scripts, like GitHub
> > Actions:
> >
> > [[[
> > However, unrelatedly to the original problem, it's much better to use
> > the Ninja generator in build scripts, since it's faster, more
> > reliable, simpler, and more customisable in terms of platforms and
> > architectures. It could be complicated to set up, but I've done it for
> > Subversion, so I can do it for Serf as well.
> > ]]]
>
> Yah. As long as the build takes 10 minutes, of which 9½ are spent
> building dependencies from source, I strongly doubt we'd gain much time
> by moving to Ninja. Given that this is Windows, we may as well use the
> "official" (heh) build tool. I just added '-j' to the builds and they're
> almost not sluggish* at all any more. If I could do a cross build and
> test of x86 Serf with MSBuild on an ARM64 Windows virtual machine, I'd
> say it's customisable enough. Frankly I don't know what you mean by
> "more reliable".
I mean that Visual Studio sometimes may work in a way no one would
expect it to. For example, that issue with openssl, where it didn't
insta-fail, and we couldn't find the source of the problem.
Also we can set the same job to build on Linux as well.
> Everything else uses generated makefiles (well, except the SCons build
> of course).
>
> -- Brane
>
> * "almost not sluggish" ... I saw that Microsoft officially supports
> clang (llvm) for ARM64, and it's "only" about 5 time faster than MSVC.
> But this is about the build tool, not the compiler.
--
Timofei Zhakov
Re: [PR] cmake imporovements [serf]
Posted by Branko Čibej <br...@apache.org>.
On 8. 6. 25 12:32, Timofei Zhakov wrote:
> On Sat, Jun 7, 2025 at 2:52 PM Branko Čibej<br...@apache.org> wrote:
>> On 7. 6. 25 14:11, rinrab (via GitHub) wrote:
>>> rinrab opened a new pull request, #6:
>>> URL:https://github.com/apache/serf/pull/6
>>>
>>> (all commits are separate changes)
>> We don't have to change the generator to achieve this; MSBuild is
>> already multi-config. The reason why I haven't made the equivalent
>> change yet is that, if debug build or test fail, the release build will
>> never happen. I'm still thinking about the ramifications of that, but I
>> for sure do not want to split the different config builds into two
>> separate jobs. Or maybe I do, I'll have to figure that out.
>>
>> -- Brane
> Yes, this is not mandatory to change the generator. But as I already
> mentioned (I think in the "GitHub actions notifications" thread),
> Ninja generator is much better for automation scripts, like GitHub
> Actions:
>
> [[[
> However, unrelatedly to the original problem, it's much better to use
> the Ninja generator in build scripts, since it's faster, more
> reliable, simpler, and more customisable in terms of platforms and
> architectures. It could be complicated to set up, but I've done it for
> Subversion, so I can do it for Serf as well.
> ]]]
Yah. As long as the build takes 10 minutes, of which 9½ are spent
building dependencies from source, I strongly doubt we'd gain much time
by moving to Ninja. Given that this is Windows, we may as well use the
"official" (heh) build tool. I just added '-j' to the builds and they're
almost not sluggish* at all any more. If I could do a cross build and
test of x86 Serf with MSBuild on an ARM64 Windows virtual machine, I'd
say it's customisable enough. Frankly I don't know what you mean by
"more reliable".
Everything else uses generated makefiles (well, except the SCons build
of course).
-- Brane
* "almost not sluggish" ... I saw that Microsoft officially supports
clang (llvm) for ARM64, and it's "only" about 5 time faster than MSVC.
But this is about the build tool, not the compiler.
Re: [PR] cmake imporovements [serf]
Posted by Timofei Zhakov <ti...@chemodax.net>.
On Sat, Jun 7, 2025 at 2:52 PM Branko Čibej <br...@apache.org> wrote:
>
> On 7. 6. 25 14:11, rinrab (via GitHub) wrote:
> > rinrab opened a new pull request, #6:
> > URL:https://github.com/apache/serf/pull/6
> >
> > (all commits are separate changes)
>
> We don't have to change the generator to achieve this; MSBuild is
> already multi-config. The reason why I haven't made the equivalent
> change yet is that, if debug build or test fail, the release build will
> never happen. I'm still thinking about the ramifications of that, but I
> for sure do not want to split the different config builds into two
> separate jobs. Or maybe I do, I'll have to figure that out.
>
> -- Brane
Yes, this is not mandatory to change the generator. But as I already
mentioned (I think in the "GitHub actions notifications" thread),
Ninja generator is much better for automation scripts, like GitHub
Actions:
[[[
However, unrelatedly to the original problem, it's much better to use
the Ninja generator in build scripts, since it's faster, more
reliable, simpler, and more customisable in terms of platforms and
architectures. It could be complicated to set up, but I've done it for
Subversion, so I can do it for Serf as well.
]]]
--
Timofei Zhakov
Re: [PR] cmake imporovements [serf]
Posted by Branko Čibej <br...@apache.org>.
On 7. 6. 25 14:11, rinrab (via GitHub) wrote:
> rinrab opened a new pull request, #6:
> URL:https://github.com/apache/serf/pull/6
>
> (all commits are separate changes)
We don't have to change the generator to achieve this; MSBuild is
already multi-config. The reason why I haven't made the equivalent
change yet is that, if debug build or test fail, the release build will
never happen. I'm still thinking about the ramifications of that, but I
for sure do not want to split the different config builds into two
separate jobs. Or maybe I do, I'll have to figure that out.
-- Brane
Re: [PR] CMake GitHub workflow imporovements [serf]
Posted by "dsahlberg-apache-org (via GitHub)" <gi...@apache.org>.
dsahlberg-apache-org commented on code in PR #6:
URL: https://github.com/apache/serf/pull/6#discussion_r2160236466
##########
.github/workflows/windows-cmake.yml:
##########
@@ -44,21 +44,29 @@ jobs:
- uses: actions/checkout@v3
+ - name: Prepare Environment (Windows)
+ if: runner.os == 'Windows'
Review Comment:
Why? Aren't we always on Windows (in this matrix)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@serf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CMake GitHub workflow imporovements [serf]
Posted by "dsahlberg-apache-org (via GitHub)" <gi...@apache.org>.
dsahlberg-apache-org commented on PR #6:
URL: https://github.com/apache/serf/pull/6#issuecomment-3146368079
I'm closing this PR. There are still some things not merged but it can be changed directly in SVN (since all participants are also committers) and if required it can be discussed on dev@.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@serf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] cmake imporovements [serf]
Posted by "rinrab (via GitHub)" <gi...@apache.org>.
rinrab commented on PR #6:
URL: https://github.com/apache/serf/pull/6#issuecomment-2953902671
Thanks!
(discussion continues on-list: https://lists.apache.org/thread/d8hc2twy612npktmwhl2gwbcb5br5tsc)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@serf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CMake GitHub workflow imporovements [serf]
Posted by "rinrab (via GitHub)" <gi...@apache.org>.
rinrab commented on PR #6:
URL: https://github.com/apache/serf/pull/6#issuecomment-2993797979
@chemodax The main reason why I decided to use Ninja **Multi-Config** is because before VC.Net generator was used to build both (debug and release) versions. So, when using Ninja Multi-Config we benefit by building the project quickly and in both configuration at the same time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@serf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CMake GitHub workflow imporovements [serf]
Posted by "chemodax (via GitHub)" <gi...@apache.org>.
chemodax commented on PR #6:
URL: https://github.com/apache/serf/pull/6#issuecomment-2993718011
I would like to suggest use Ninja (not multi-config generator) for Windows build:
1. It's fast.
2. It's default.
3. This is how vcpkg and other typical consumer will use Serf.
Just my $0.02.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@serf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CMake GitHub workflow imporovements [serf]
Posted by Branko Čibej <br...@apache.org>.
On 22. 6. 25 02:11, brainy (via GitHub) wrote:
> brainy commented on PR #6:
> URL:https://github.com/apache/serf/pull/6#issuecomment-2993831947
>
> > (GitHub merge completely destroyed the branch for some reason, and I didn't initially noticed that. So commits are a bit messed up. Sorry :-S)
>
> I propose to close this pull request and go back to fundamentals, i.e., discussing things on-list.
So please, let's start a new thread here. I'd like to see a discussion
of what is supposedly the problem before throwing code into a PR.
-- Brane
Re: [PR] CMake GitHub workflow imporovements [serf]
Posted by "brainy (via GitHub)" <gi...@apache.org>.
brainy commented on PR #6:
URL: https://github.com/apache/serf/pull/6#issuecomment-2993831947
> (GitHub merge completely destroyed the branch for some reason, and I didn't initially noticed that. So commits are a bit messed up. Sorry :-S)
I propose to close this pull request and go back to fundamentals, i.e., discussing things on-list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@serf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CMake GitHub workflow imporovements [serf]
Posted by "dsahlberg-apache-org (via GitHub)" <gi...@apache.org>.
dsahlberg-apache-org closed pull request #6: CMake GitHub workflow imporovements
URL: https://github.com/apache/serf/pull/6
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@serf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] cmake imporovements [serf]
Posted by "rinrab (via GitHub)" <gi...@apache.org>.
rinrab commented on PR #6:
URL: https://github.com/apache/serf/pull/6#issuecomment-2993620179
@dsahlberg-apache-org, this PR also contains Ninja usage in the github workflow which wasn't committed yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@serf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] cmake imporovements [serf]
Posted by "brainy (via GitHub)" <gi...@apache.org>.
brainy commented on PR #6:
URL: https://github.com/apache/serf/pull/6#issuecomment-2953616283
Something similar is now on trunk, r1926240:1926243.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@serf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CMake GitHub workflow imporovements [serf]
Posted by "brainy (via GitHub)" <gi...@apache.org>.
brainy commented on PR #6:
URL: https://github.com/apache/serf/pull/6#issuecomment-2993831073
> I think this was discussed on the list. I can't really judge the implications of one over the other. It was mentioned onlist where @rinrab had some arguments in favour of moving to Ninja.
>
> @brainy Do you have a strong feeling about keeping VS?
Guys, slow down. I keep hearing this "Ninja is faster" argument, but has anyone bothered to actually **look** at what's making the Windows build so slow? Hint: it's not MSBuild.
Besides, there's another, more fundamental problem with the CMake build: the debug build doesn't use the debug-mode dependencies that were installed by vcpkg, and consequently, a test is failing. 10 internet points to anyone who tells me why (I know the answer). This is a direct consequence of CMake being fundamentally broken at the architecture level. No amount of using different generators will fix that.
To summarize, messing around with Ninja versus VS does exactly nothing to improve the build.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@serf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CMake GitHub workflow imporovements [serf]
Posted by "dsahlberg-apache-org (via GitHub)" <gi...@apache.org>.
dsahlberg-apache-org commented on PR #6:
URL: https://github.com/apache/serf/pull/6#issuecomment-2993636263
I think this was discussed on the list. I can't really judge the implications of one over the other. It was mentioned onlist where @rinrab had some arguments in favour of moving to Ninja.
@brainy Do you have a strong feeling about keeping VS?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@serf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CMake GitHub workflow imporovements [serf]
Posted by "chemodax (via GitHub)" <gi...@apache.org>.
chemodax commented on code in PR #6:
URL: https://github.com/apache/serf/pull/6#discussion_r2160171234
##########
.github/workflows/windows-cmake.yml:
##########
@@ -44,21 +44,29 @@ jobs:
- uses: actions/checkout@v3
+ - name: Prepare Environment (Windows)
+ if: runner.os == 'Windows'
+ run: |
+ $root = "C:\Program Files\Microsoft Visual Studio\2022\Enterprise"
+ Import-Module "$root\Common7\Tools\Microsoft.VisualStudio.DevShell.dll"
+ Enter-VsDevShell -VsInstallPath $root -DevCmdArguments "-arch=${{ matrix.arch }}"
+
+ ls env: | foreach { "$($_.Name)=$($_.Value)" >> $env:GITHUB_ENV }
+
- name: Configure CMake
run: >
cmake
-B ${{github.workspace}}/build
-G "${{ matrix.generator }}"
- -DCMAKE_GENERATOR_PLATFORM=${{ matrix.platform }}
-DCMAKE_TOOLCHAIN_FILE=C:/vcpkg/scripts/buildsystems/vcpkg.cmake
-DCMAKE_PREFIX_PATH=C:/vcpkg/installed/${{ matrix.triplet }}
-DCMAKE_INSTALL_PREFIX=${{github.workspace}}/install
- name: Build (Release)
- run: cmake --build ${{github.workspace}}/build --config Release -j
Review Comment:
These changes seems to be unrelated to the pull request (while I support the change itself).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@serf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CMake GitHub workflow imporovements [serf]
Posted by "rinrab (via GitHub)" <gi...@apache.org>.
rinrab commented on PR #6:
URL: https://github.com/apache/serf/pull/6#issuecomment-2993798759
(GitHub merge completely destroyed the branch for some reason, and I didn't initially noticed that. So commits are a bit messed up. Sorry :-S)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@serf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] cmake imporovements [serf]
Posted by "dsahlberg-apache-org (via GitHub)" <gi...@apache.org>.
dsahlberg-apache-org commented on PR #6:
URL: https://github.com/apache/serf/pull/6#issuecomment-2993617465
@rinrab, are you happy with r1926240:1926243, ie: should we close this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@serf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org