Skip to content

Conversation

@JMD-tech
Copy link

@JMD-tech JMD-tech commented Jun 11, 2025

Hello,
Worked some more on porting. Got rid of all but one compile warning for MinGW.
update: fixed.

Here's the remaining warning:

src/internal_modules/roc_address/target_berkley/roc_address/socket_addr.cpp: In member function ‘bool roc::address::SocketAddr::is_multicast() const’:
src/internal_modules/roc_address/target_berkley/roc_address/socket_addr.cpp:150:16: warning: conversion to ‘long unsigned int’ from ‘long int’ may change the sign of the result [-Wsign-conversion]
  150 |         return IN_MULTICAST(core::ntoh32u(saddr_.addr4.sin_addr.s_addr));
      |                ^~~~~~~~~~~~

Tried about any kind of casts to no avail, and was surprised to see it was the other way around, as ntohs uint32_t return that would be cast to signed in the macro, also probably the macro expansion made the compiler point to "function" macro instead of parameter, but same attempts with casts of parameter in in function param were also no avail.
Same as some attempts as replacing with !=0 and similar.


Now i suppose i i'd have only this one to be solved to consider those modifications to wothy of merge?
Marking it as ready for review, and will start working on (supposed agree on priorities for MinGW32/Windows):

  • (2) implement WIN32 native functions
  • enable OpenFEC (well, not that a priority but as it's a single line of code away...)
  • build roc-recv/roc-send tools

gh-292

Ok, i think it's ready for review, removing the 'draft mark'...

With all of those (plus copy of some files from posix) it successfully generates libroc_*.a:

  • (generic, all builds) new URL for libatomic_ops releases
  • socket_addr & socket_ops modifications
  • include errno_to_str.h from wav_sink/source.cpp
  • minor fixes in windows cond.cpp and errno_to_str (local var name mismatch, and const attribute conflicting)
  • getting MinGW build to compile without -fpermissive:
    Minimal MinGW build for Windows #292 (comment)
  • doing the socket_init/deinit() in Context c/dtor

Meanwhile i'll try to static-link this libroc_core.a in Windows roc-send, and try to cross-compile roc-recv/send to see if it works.
Then i think as next priorities:

  • clear the last few remaining warnings when compiling for MinGW
  • working on item (2) porting posix code to native Windows API

@rocstreaming-bot rocstreaming-bot added the contrib PR not by a maintainer label Jun 11, 2025
@rocstreaming-bot
Copy link

🤖 Pull request description does not have a link to an issue.
If there is a related issue, please add it to the description using any of the supported formats.

@rocstreaming-bot rocstreaming-bot added the S-ready-for-review status: PR can be reviewed label Jun 11, 2025
@JMD-tech
Copy link
Author

JMD-tech commented Jun 11, 2025

Ok, learning to use PR's, i believed it would contain only the commit for the URL, but looks like the PR includes all commits including ones added later, so it contains also the socket_addr and _ops which i intended to put on a separate PR...

Now i get all CI tests pass, i'll continue working on local machine, as i suppose for incremental approach you prefer that i don't push anymore stuff into this PR yet, and wait for this one to be reviewed and maybe fixed some more.

@JMD-tech JMD-tech changed the title Use new URL for libatomic_ops releases Early fixes for MinGW build Jun 12, 2025
@JMD-tech JMD-tech marked this pull request as draft June 13, 2025 15:21
@rocstreaming-bot rocstreaming-bot added S-work-in-progress status: PR is still in progress and changing and removed S-ready-for-review status: PR can be reviewed labels Jun 13, 2025
@JMD-tech JMD-tech changed the title Early fixes for MinGW build Early fixes for MinGW build - draft PR Jun 14, 2025
@gavv
Copy link
Member

gavv commented Jun 15, 2025

Hi, thanks for update!

I'll checkout changes next week, here are just 2 small remarks:

  1. Current CI failure is unrelated to PR, you can rebase on fresh develop to fix it.

  2. Seems that troubles with formatting are caused by this poor script, not by clang-format ("scons fmt" invokes both things).

    I always wanted to rewrite it, so I did it now: 4d4abbb

    I hope rebasing on develop will fix your issues and you won't need dummy comments.

    If I'm wrong and some problems are caused by clang-format, you can also use clang-format magic comment:

    // clang-format off
    <something>
    // clang-format on

@gavv
Copy link
Member

gavv commented Jun 15, 2025

and once i've got it right, i think you'll agree it's better that i do another PR that would be equivalent in files modified but without extra commits like fixing things in previous ones to avoid cluttering branches with those ("playing back" the final modifications, and hopefully have all intermediate commits validate CI checks).

I'm fine with this approach, just FYI:

  • When merging PR, github allows to squash all commits into one, so it's not really necessary to create a separate PR just for squashing.

    (Actually I use my own script for merging PRs, but it behaves the same way).

  • If you force-push to your PR's branch, github will automatically update PR, so it's also not necessary to create a new PR if you want to edit history by hand.

    (Github PR is tied to fork name + branch name, not specific commit).

@JMD-tech JMD-tech marked this pull request as draft July 7, 2025 01:32
@rocstreaming-bot rocstreaming-bot added S-work-in-progress status: PR is still in progress and changing and removed S-needs-revision status: Author should revise PR and address feedback labels Jul 7, 2025
@JMD-tech
Copy link
Author

Hello,
Worked some more on porting. Got rid of all but one compile warning for MinGW.

Here's the remaining warning:

src/internal_modules/roc_address/target_berkley/roc_address/socket_addr.cpp: In member function ‘bool roc::address::SocketAddr::is_multicast() const’:
src/internal_modules/roc_address/target_berkley/roc_address/socket_addr.cpp:150:16: warning: conversion to ‘long unsigned int’ from ‘long int’ may change the sign of the result [-Wsign-conversion]
  150 |         return IN_MULTICAST(core::ntoh32u(saddr_.addr4.sin_addr.s_addr));
      |                ^~~~~~~~~~~~

Tried about any kind of casts to no avail, and was surprised to see it was the other way around, as ntohs uint32_t return that would be cast to signed in the macro, also probably the macro expansion made the compiler point to "function" macro instead of parameter, but same attempts with casts of parameter in in function param were also no avail.
Same as some attempts as replacing with !=0 and similar.

Now i suppose i i'd have only this one to be solved to consider those modifications to wothy of merge?
Marking it as ready for review, and will start working on (supposed agree on priorities for MinGW32/Windows):

  • (2) implement WIN32 native functions
  • enable OpenFEC (well, not that a priority but as it's a single line of code away...)
  • build roc-recv/roc-send tools

@JMD-tech JMD-tech requested a review from gavv July 23, 2025 00:03
@rocstreaming-bot rocstreaming-bot added S-ready-for-review status: PR can be reviewed and removed S-work-in-progress status: PR is still in progress and changing labels Jul 23, 2025
@JMD-tech JMD-tech marked this pull request as ready for review August 4, 2025 22:11
@JMD-tech JMD-tech marked this pull request as draft October 4, 2025 18:44
@rocstreaming-bot rocstreaming-bot added S-work-in-progress status: PR is still in progress and changing and removed S-ready-for-review status: PR can be reviewed labels Oct 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contrib PR not by a maintainer S-work-in-progress status: PR is still in progress and changing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants