Page MenuHomeWildfire Games

Fix some more issues with the RL interface
ClosedPublic

Authored by Stan on Jan 16 2021, 5:51 PM.

Details

Summary

See Phab:rP24631

Test Plan

Review.

Event Timeline

Stan created this revision.Jan 16 2021, 5:51 PM
vladislavbelov added inline comments.Jan 16 2021, 5:55 PM
source/rlinterface/RLInterface.cpp
166–167

Is it guaranteed that the buf contains a valid C-string (ended with \0)?

Build has FAILED

builderr-debug-macos.txt
fatal error: file '/Users/wfg/Jenkins/workspace/macos-differential/build/workspaces/gcc/../../../libraries/source/spidermonkey/include-unix-debug/mozilla/LinkedList.h' has been modified since the precompiled header 'obj/engine_Debug/precompiled.h.gch' was built
note: please rebuild precompiled header 'obj/engine_Debug/precompiled.h.gch'
1 error generated.
make[1]: *** [obj/engine_Debug/JSInterface_Game.o] Error 1
make: *** [engine] Error 2

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/2899/display/redirect
See console output for more information: https://jenkins.wildfiregames.com/job/macos-differential/2899/display/redirectconsole

Stan requested review of this revision.Jan 16 2021, 7:19 PM
Stan added inline comments.Jan 17 2021, 4:14 PM
source/rlinterface/RLInterface.cpp
166–167

From https://github.com/0ad/0ad/blob/764dd1527b9ee97acf70680ecb98a951d9a5c82b/source/third_party/mongoose/mongoose.cpp#L1400 there doesn't seem to be any check, but you asked @irishninja to do it so I assumed you knew?

irishninja updated this revision to Diff 15763.Jan 29 2021, 4:27 AM
  • Refactor reading HTTP request content logic

Build has FAILED

builderr-debug-macos.txt
../../../source/rlinterface/RLInterface.cpp:152:38: error: call to unavailable member function 'value': introduced in macOS 10.14
                        scenario.content = std::move(data.value());
                                                     ~~~~~^~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/optional:933:33: note: candidate function has been explicitly made unavailable
    constexpr value_type const& value() const&
                                ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/optional:942:27: note: candidate function not viable: 'this' argument has type 'const std::optional<std::string>' (aka 'const optional<basic_string<char, char_traits<char>, allocator<char> > >'), but method is not marked const
    constexpr value_type& value() &
                          ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/3114/display/redirect
See console output for more information: https://jenkins.wildfiregames.com/job/macos-differential/3114/display/redirectconsole

irishninja added inline comments.Jan 29 2021, 4:31 AM
source/rlinterface/RLInterface.cpp
166–167

It doesn't look like it since [[ https://github.com/0ad/0ad/blob/764dd1527b9ee97acf70680ecb98a951d9a5c82b/source/third_party/mongoose/mongoose.cpp#L1394 | pull may use recv ]] which has no such guarantee but I could be missing something... I could update the function that I just added to ensure the string ends with a \0. Let me know what you are thinking!

std::optional isn't available for us because we're compiling on mac using Xcode 10: https://trac.wildfiregames.com/wiki/CppSupport.
Your options include checking for empty string, or returning boolean and passing a ref-parameter

irishninja updated this revision to Diff 15774.Jan 30 2021, 4:56 AM
  • Remove std::optional for OSX support
  • Fixed string initialization

Build is green

builderr-debug-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine_dbg.a(precompiled.o) has no symbols
ld: warning: text-based stub file /System/Library/Frameworks//CoreAudio.framework/CoreAudio.tbd and library file /System/Library/Frameworks//CoreAudio.framework/CoreAudio are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox.tbd and library file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback.tbd and library file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//CoreVideo.framework/Cor

See https://jenkins.wildfiregames.com/job/macos-differential/3122/display/redirect for more details.

Thanks, @wraitii. Bummer about not being able to use optional. I updated it!

source/rlinterface/RLInterface.cpp
166–167

After looking at it a little bit more, it should not be an issue since data() automatically appends a null character (ie, the contentSize + 1 th character) and we are only reading contentSize characters from the socket/file.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 5 2021, 1:23 PM
This revision was automatically updated to reflect the committed changes.