Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve Windows (MinGW) issues and minor improvements #47

Merged
merged 11 commits into from
Apr 12, 2021

Conversation

Broekman
Copy link
Collaborator

@Broekman Broekman commented Apr 7, 2021

Refer to #46 for more information.

@jadamroth
Copy link
Collaborator

Thank you. Please give me a few days to review

@jadamroth
Copy link
Collaborator

Sorry for the delay. I just found time to look at this.

For Linux (arm), I'm receiving compile errors (see below) relating to your EIPSCANNER_SOCKET_ERROR in sockets/Platform.h

/home/adam/Downloads/EIPScanner/src/sockets/Platform.h:10:38: error: ‘WSAEINPROGRESS’ was not declared in this scope
 #define EIPSCANNER_SOCKET_ERROR(err) WSA##err
                                      ^~~
/home/adam/Downloads/EIPScanner/src/sockets/TCPSocket.cpp:58:39: note: in expansion of macro ‘EIPSCANNER_SOCKET_ERROR’
     if (BaseSocket::getLastError() == EIPSCANNER_SOCKET_ERROR(EINPROGRESS)) {
                                       ^~~~~~~~~~~~~~~~~~~~~~~
/home/adam/Downloads/EIPScanner/src/sockets/Platform.h:10:38: note: suggested alternative: ‘EINPROGRESS’
 #define EIPSCANNER_SOCKET_ERROR(err) WSA##err
                                      ^~~
/home/adam/Downloads/EIPScanner/src/sockets/TCPSocket.cpp:58:39: note: in expansion of macro ‘EIPSCANNER_SOCKET_ERROR’
     if (BaseSocket::getLastError() == EIPSCANNER_SOCKET_ERROR(EINPROGRESS)) {
                                       ^~~~~~~~~~~~~~~~~~~~~~~
/home/adam/Downloads/EIPScanner/src/sockets/Platform.h:10:38: error: ‘WSAEINTR’ was not declared in this scope
 #define EIPSCANNER_SOCKET_ERROR(err) WSA##err
                                      ^~~
/home/adam/Downloads/EIPScanner/src/sockets/TCPSocket.cpp:67:52: note: in expansion of macro ‘EIPSCANNER_SOCKET_ERROR’
       if (res < 0 && BaseSocket::getLastError() != EIPSCANNER_SOCKET_ERROR(EINTR)) {
                                                    ^~~~~~~~~~~~~~~~~~~~~~~
/home/adam/Downloads/EIPScanner/src/sockets/Platform.h:10:38: note: suggested alternative: ‘EINTR’
 #define EIPSCANNER_SOCKET_ERROR(err) WSA##err
                                      ^~~
/home/adam/Downloads/EIPScanner/src/sockets/TCPSocket.cpp:67:52: note: in expansion of macro ‘EIPSCANNER_SOCKET_ERROR’
       if (res < 0 && BaseSocket::getLastError() != EIPSCANNER_SOCKET_ERROR(EINTR)) {
                                                    ^~~~~~~~~~~~~~~~~~~~~~~
/home/adam/Downloads/EIPScanner/src/sockets/Platform.h:10:38: error: ‘WSAETIMEDOUT’ was not declared in this scope
 #define EIPSCANNER_SOCKET_ERROR(err) WSA##err
                                      ^~~
/home/adam/Downloads/EIPScanner/src/sockets/TCPSocket.cpp:82:32: note: in expansion of macro ‘EIPSCANNER_SOCKET_ERROR’
        throw std::system_error(EIPSCANNER_SOCKET_ERROR(ETIMEDOUT), BaseSocket::getErrorCategory());
                                ^~~~~~~~~~~~~~~~~~~~~~~
/home/adam/Downloads/EIPScanner/src/sockets/Platform.h:10:38: note: suggested alternative: ‘ETIMEDOUT’
 #define EIPSCANNER_SOCKET_ERROR(err) WSA##err
                                      ^~~
/home/adam/Downloads/EIPScanner/src/sockets/TCPSocket.cpp:82:32: note: in expansion of macro ‘EIPSCANNER_SOCKET_ERROR’
        throw std::system_error(EIPSCANNER_SOCKET_ERROR(ETIMEDOUT), BaseSocket::getErrorCategory());
                                ^~~~~~~~~~~~~~~~~~~~~~~
src/CMakeFiles/EIPScannerS.dir/build.make:348: recipe for target 'src/CMakeFiles/EIPScannerS.dir/sockets/TCPSocket.cpp.o' failed
make[2]: *** [src/CMakeFiles/EIPScannerS.dir/sockets/TCPSocket.cpp.o] Error 1
CMakeFiles/Makefile2:94: recipe for target 'src/CMakeFiles/EIPScannerS.dir/all' failed
make[1]: *** [src/CMakeFiles/EIPScannerS.dir/all] Error 2
Makefile:129: recipe for target 'all' failed
make: *** [all] Error 2

I'm also receiving compile errors on MacOS relating to endpoint.h

  1. Will you please add the preprocessor || defined(__APPLE__) in Endpoint.h line 8 as is in the current repo. This will fix compile errors, I'm receiving
  2. Please add the same preprocessor at the top of BaseSocket.cpp line 5
  3. Same thing in Endpoint.cpp lines 8 and 35
  4. Same thing in TCPSocket.cpp line 5
  5. Once the processors are added, I get the same compile errors as in Linux with the Platform.h errors

Once these errors are resolved and the app properly compiles, I'll work more towards testing the new changes on both platforms. Thanks again for the pull request

@Broekman
Copy link
Collaborator Author

No problem, I'll have a look later today. I also compiled (and tested) on Arch Linux (x64) and had no issues for now, strange that MAC doesn't respond to the unix def.

@Broekman
Copy link
Collaborator Author

I stand corrected, the updates/cleaning I did later broke the build on Linux. Moved the #if defined too far down in Platform.h. I also thought that unix also captured both linux and apple but seems not to be the case. Added it again and updated the platform file. It now compiles again on my Linux machine, will test a bit more (and see why Travis build fails).

@jadamroth
Copy link
Collaborator

Thanks for the update.

Once your changes are merged, I'll need to get a windows travis server up and running. I'd imagine there will be enough future usage for windows applications to justify. I've never tested multiple OS's in one CI server for a single repo, but I'd assume this is a relatively common task.

I don't know if there's enough use on macos to justify it's own build server, I just use it for miscellaneous testing, and most things that work for linux work similarly as mac.

@Broekman
Copy link
Collaborator Author

Ah, "static constexpr" has become explicitly inline >=C++17, not yet in 14 (causes a linker error). I wasn't getting these on my end as I'm compiling with C++20. Will fix in a bit after which you can re-check. Thanks!

@Broekman
Copy link
Collaborator Author

Ok all updated, just briefly tested discovery, implicit and explicit messaging on my Linux (and Windows) machines, looks good.

@jadamroth
Copy link
Collaborator

Thank you. I will test today on linux and mac and follow up with you

@jadamroth
Copy link
Collaborator

For mac, please add preprocessor || defined(__APPLE__) on

  1. DiscoveryManager.cpp line 15
  2. BaseSocket.cpp line 5

With those changes, mac works. Testing linux

@Broekman
Copy link
Collaborator Author

Oops, my "find and replace all" skills are lacking, sorry, done.

@jadamroth
Copy link
Collaborator

I tested with Linux (arm), and it works. I don't have the ability to test linux x64, but I'd assume this library works the same as arm.

I'm going to go ahead and merge to master and create a new release 1.2.0. If there's any bugs, we'll build a new release from there (1.2.1 and so on).

Thanks for your pull request and for using the library. If you plan on making a lot of changes in the future, let me know, and I can give you write access

@jadamroth jadamroth merged commit c1fa5c1 into nimbuscontrols:master Apr 12, 2021
@Broekman
Copy link
Collaborator Author

Thanks, I will be using the library more heavily this year, probably also in a larger industrial setting for support tooling on our embedded controllers or for simulation software. So access for the future might come in handy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants