Page MenuHomeWildfire Games
Differential D3183

Redo D2399
ClosedPublic
Actions

Authored by Stan on Dec 4 2020, 8:30 PM.

Details

Reviewers
wraitii
nephele
Itms
Commits
rP24337: Fix compilation on FreeBSD by removing the evct dependency.
Trac Tickets
#1325
Summary

Title

Test Plan

Test it works on all oses.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Stan created this revision.Dec 4 2020, 8:30 PM
Herald added a reviewer: Itms. · View Herald TranscriptDec 4 2020, 8:31 PM
Vulcan added a comment.Dec 4 2020, 8:50 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/2275/display/redirect

Vulcan added a comment.Dec 4 2020, 9:37 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/3939/display/redirect

Harbormaster completed remote builds in B14146: Diff 14396.Dec 4 2020, 9:37 PM
Stan requested review of this revision.Dec 4 2020, 9:37 PM
lyv added a subscriber: lyv.Dec 4 2020, 9:44 PM
lyv added inline comments.
build/premake/extern_libs5.lua
248 ↗(On Diff #14396)

This is getting out of hand now. Maybe my own taste, so consider that too I guess.

My suggestion for this is either use the standard library, or wrap the ad-hoc implementation in a platform directive.

But strongly the first one. This quite simple issue is quickly becoming overwhelming.

nephele added inline comments.Dec 4 2020, 9:49 PM
build/premake/extern_libs5.lua
248 ↗(On Diff #14396)

My suggestion for this is either use the standard library, or wrap the ad-hoc implementation in a platform directive.

Posix has deprecated this symbol, hence the test. there is no standard library to use on some platforms.

We already do this for backtrack symbols from execinfo that glibc provides but other libc's don't, where is the difference here?

The other option would be to ifdef for FreeBSD for now and keep ifdeffing for any other platform that has removed ecve or does so in the future, doing it once with a detection seems, to me, to be a more proper solution. just my 2 cents though.

lyv added inline comments.Dec 4 2020, 9:59 PM
build/premake/extern_libs5.lua
248 ↗(On Diff #14396)

I mean isocpp standard which if the compiler cannot support can;t build the the project anyway. Again, I don't understand why using a decade old c++ standard is not the preferred alternative rather than trying to do things the C way.

Posix has deprecated this symbol, hence the test. there is no standard library to use on some platforms.

Hence more reason to not carry that baggage around anymore. There is probably a reason why POSIX deprecated it. This is C++, so use the C++ libraries.

We already do this for backtrack symbols from execinfo that glibc provides but other libc's don't, where is the difference here?

I don't like that either. That too is non standard and availble in glibc. Why not just #if defined(__GNU_LIBRARY__)?.

nephele added inline comments.Dec 4 2020, 10:04 PM
build/premake/extern_libs5.lua
248 ↗(On Diff #14396)

Hence more reason to not carry that baggage around anymore. There is probably a reason why POSIX deprecated it. This is C++, so use the C++ libraries.

Are you suggesting to rewrite the calls to ecvt instead? that would work better i suppose.

I don't like that either. That too is non standard and availble in glibc. Why not just #if defined(GNU_LIBRARY)

Nobody suggested it at the time, that might work too, if you would like to change that?

lyv added a comment.Dec 4 2020, 10:05 PM

(In case I sound passive aggressive, that is not the intention here. I just talk really directly.)

wraitii added a comment.Dec 5 2020, 8:22 AM

Have to say -> changing the one function to not use ecvt but something else seems a much better solution.

Stan planned changes to this revision.Dec 5 2020, 9:23 AM

Need to rethink everything.

Stan updated this revision to Diff 14402.Dec 5 2020, 2:28 PM

Better fix, in c++

Stan updated this revision to Diff 14403.Dec 5 2020, 2:28 PM

Better fix, in c++

Vulcan added a comment.Dec 5 2020, 2:29 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/2280/display/redirect

Vulcan added a comment.Dec 5 2020, 2:30 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/2281/display/redirect

Stan updated this revision to Diff 14404.Dec 5 2020, 2:31 PM

Add missing <limits>

Vulcan added a comment.Dec 5 2020, 2:34 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/2282/display/redirect

wraitii added a comment.Dec 5 2020, 2:49 PM

I feel like the regex is kind of overkill still?

libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilderTest.cpp
55 ↗(On Diff #14404)

why is this one deleted?

Vulcan added a comment.Dec 5 2020, 2:52 PM

Successful build - Chance fights ever on the side of the prudent.

builderr-release-gcc7.txt
In file included from ../../../source/lib/os_path.h:26,
                 from ../../../source/lib/self_test.h:28,
                 from ../../../source/simulation2/system/ComponentTest.h:21,
                 from ../../../source/pch/test/precompiled.h:22:
../../../source/lib/path.h: In member function 'virtual void TestMapGenerator::setUp()':
../../../source/lib/path.h:264:68: warning: '<anonymous>.Path::separator' may be used uninitialized in this function [-Wmaybe-uninitialized]
   debug_printf("Path %s, separator %c\n", string8().c_str(), (char)separator);
                                                                    ^~~~~~~~~
In file included from ../../../source/lib/os_path.h:26,
                 from ../../../source/lib/self_test.h:28,
                 from ../../../source/simulation2/system/ComponentTest.h:21,
                 from ../../../source/pch/test/precompiled.h:22:
../../../source/lib/path.h: In member function 'virtual void TestComponentScripts::setUp()':
../../../source/lib/path.h:264:68: warning: '<anonymous>.Path::separator' may be used uninitialized in this function [-Wmaybe-uninitialized]
   debug_printf("Path %s, separator %c\n", string8().c_str(), (char)separator);
                                                                    ^~~~~~~~~
In file included from ../../../source/lib/os_path.h:26,
                 from ../../../source/lib/self_test.h:28,
                 from ../../../source/simulation2/system/ComponentTest.h:21,
                 from ../../../source/pch/test/precompiled.h:22:
../../../source/lib/path.h: In function 'OsPath DataDir()':
../../../source/lib/path.h:264:68: warning: '<anonymous>.Path::separator' may be used uninitialized in this function [-Wmaybe-uninitialized]
   debug_printf("Path %s, separator %c\n", string8().c_str(), (char)separator);
                                                                    ^~~~~~~~~

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/3943/display/redirect

Harbormaster completed remote builds in B14150: Diff 14402.Dec 5 2020, 2:52 PM
Vulcan added a comment.Dec 5 2020, 2:53 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/3944/display/redirect

Harbormaster completed remote builds in B14151: Diff 14403.Dec 5 2020, 2:53 PM
Vulcan added a comment.Dec 5 2020, 2:54 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/3945/display/redirect

Harbormaster completed remote builds in B14152: Diff 14404.Dec 5 2020, 2:54 PM
Stan updated this revision to Diff 14407.Dec 5 2020, 3:02 PM

Add missing semicolon

Vulcan added a comment.Dec 5 2020, 3:04 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/2284/display/redirect

Stan marked an inline comment as done.Dec 5 2020, 3:23 PM
Stan added inline comments.
libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilderTest.cpp
55 ↗(On Diff #14404)

As per my comment on IRC today. Previously evct would crop numbers this one was turned into "102315" as per the 6 digits precision in FloatToString. While it makes sense for exponents, I don't think it does for plain numbers like this one.

Stan updated this revision to Diff 14408.Dec 5 2020, 5:03 PM
Stan marked an inline comment as done.

More fixes suggested by wraitii

Vulcan added a comment.Dec 5 2020, 5:04 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/2285/display/redirect

Vulcan added a comment.Dec 5 2020, 5:07 PM

Successful build - Chance fights ever on the side of the prudent.

builderr-release-gcc7.txt
In file included from ../../../source/lib/os_path.h:26,
                 from ../../../source/lib/self_test.h:28,
                 from ../../../source/simulation2/system/ComponentTest.h:21,
                 from ../../../source/pch/test/precompiled.h:22:
../../../source/lib/path.h: In member function 'virtual void TestMapGenerator::setUp()':
../../../source/lib/path.h:264:68: warning: '<anonymous>.Path::separator' may be used uninitialized in this function [-Wmaybe-uninitialized]
   debug_printf("Path %s, separator %c\n", string8().c_str(), (char)separator);
                                                                    ^~~~~~~~~
In file included from ../../../source/lib/os_path.h:26,
                 from ../../../source/lib/self_test.h:28,
                 from ../../../source/simulation2/system/ComponentTest.h:21,
                 from ../../../source/pch/test/precompiled.h:22:
../../../source/lib/path.h: In member function 'virtual void TestComponentScripts::setUp()':
../../../source/lib/path.h:264:68: warning: '<anonymous>.Path::separator' may be used uninitialized in this function [-Wmaybe-uninitialized]
   debug_printf("Path %s, separator %c\n", string8().c_str(), (char)separator);
                                                                    ^~~~~~~~~
In file included from ../../../source/lib/os_path.h:26,
                 from ../../../source/lib/self_test.h:28,
                 from ../../../source/simulation2/system/ComponentTest.h:21,
                 from ../../../source/pch/test/precompiled.h:22:
../../../source/lib/path.h: In function 'OsPath DataDir()':
../../../source/lib/path.h:264:68: warning: '<anonymous>.Path::separator' may be used uninitialized in this function [-Wmaybe-uninitialized]
   debug_printf("Path %s, separator %c\n", string8().c_str(), (char)separator);
                                                                    ^~~~~~~~~

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/3947/display/redirect

Harbormaster completed remote builds in B14155: Diff 14407.Dec 5 2020, 5:07 PM
Stan updated this revision to Diff 14409.Dec 5 2020, 5:16 PM

More fixes suggested by wraitii

Vulcan added a comment.Dec 5 2020, 5:17 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/2286/display/redirect

Stan updated this revision to Diff 14410.Dec 5 2020, 5:17 PM

Update other includes

Vulcan added a comment.Dec 5 2020, 5:17 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/2287/display/redirect

Harbormaster completed remote builds in B14156: Diff 14408.Dec 6 2020, 9:01 AM
Harbormaster completed remote builds in B14157: Diff 14409.Dec 6 2020, 9:13 AM
Harbormaster completed remote builds in B14158: Diff 14410.Dec 6 2020, 9:27 AM
Stan mentioned this in rP24336: Make unicode builds work again for FCollada refs Phab:D3183.Dec 6 2020, 7:34 PM
This revision was not accepted when it landed; it landed in state Needs Review.Dec 6 2020, 7:44 PM
This revision was landed with ongoing or failed builds.
Closed by commit rP24337: Fix compilation on FreeBSD by removing the evct dependency. (authored by Stan). · Explain Why
This revision was automatically updated to reflect the committed changes.
Stan added a commit: rP24337: Fix compilation on FreeBSD by removing the evct dependency..
Owners added a subscriber: Restricted Owners Package.Dec 6 2020, 7:44 PM

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
system/
libraries/
source/
fcollada/
2 lines
include/
FUtils/
10 lines
142 lines
lib/
src/
FCollada/
FUtils/
10 lines
142 lines
5 lines
12 lines

Diff 14432

View Options

ps/trunk/binaries/system/FCollada.dll

Loading...
View Options

ps/trunk/binaries/system/FCollada.pdb

Loading...
View Options

ps/trunk/binaries/system/FColladaD.dll

Loading...
View Options

ps/trunk/binaries/system/FColladaD.pdb

Loading...
View Options

ps/trunk/libraries/source/fcollada/build.sh

Loading...
View Options

ps/trunk/libraries/source/fcollada/include/FUtils/FUStringBuilder.h

Loading...
View Options

ps/trunk/libraries/source/fcollada/include/FUtils/FUStringBuilder.hpp

Loading...
View Options

ps/trunk/libraries/source/fcollada/lib/FColladaD.lib

Loading...
View Options

ps/trunk/libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.h

Loading...
View Options

ps/trunk/libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.hpp

Loading...
View Options

ps/trunk/libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilder.cpp

Loading...
View Options

ps/trunk/libraries/source/fcollada/src/FCollada/FUtils/FUStringBuilderTest.cpp

Loading...
Wildfire Games · Phabricator