Build broken with --without-pch flag to update-workspaces.sh.
Details
Details
- Reviewers
vladislavbelov elexis - Group Reviewers
Restricted Owners Package (Owns No Changed Paths)
Build with and without the patch (or check that there are maps in that header,
and none of the included files includes that).
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- compilation_fix
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 1288 Build 2028: Vulcan Build Jenkins Build 2027: arc lint + arc unit
Event Timeline
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/891/ for more details.
Comment Actions
In future, we should remove the map from here, because any path already know own name.
It'd be like:
SetPaths(const std::vector<CCinemaPath>& path); // or list
Comment Actions
Stop, #include <list> should be there too? At least, we returns it. If we'll remove some headers which already have a list, then there'd be an error.
Comment Actions
Yes, but it does compile for me without it and with --without-pch.
We most likely should, but that can be done later. This is just about actually fixing the build.
Comment Actions
Yes, but the error may appear again when someone remove #include <list> in one of dependencies.
Comment Actions
I think headers should include all needed headers, and not include a header, if it'll be obviously included by an other header.
Comment Actions
I think headers should include all needed headers, and not include a header, if it'll be obviously included by an other header.
They should include what is needed, not more. But mostly I'd like to fix the build.
If all files that included the thing we need here would be rendered obsolete and removed, then that inclusion would have to be added to this file to fix, the build again right?
Comment Actions
If all files that included the thing we need here would be rendered obsolete and removed, then that inclusion would have to be added to this file to fix, the build again right?
Yes, if someone changes other code that breaks unrelated code that change should include the fix to that breakage.
But the whole point of this is that the current code is broken with that flag (and unless someone wants to drop support for that this change will have to be included at some point before a release). I could have just raised an issue with the first commit that broke this, but fixing it was less work than finding out which commit that was.
Comment Actions
But it's the fault of the the cinematics code if the other file rightfully removed an inclusion and the cinematics code fails to build because it missed to include all necessities (and this proposal was about adding those that we noticed absent, right?)
But the whole point of this is that the current code is broken with that flag (and unless someone wants to drop support for that this change will have to be included at some point before a release).
We all agree with the point to fix this
I could have just raised an issue with the first commit that broke this, but fixing it was less work than finding out which commit that was.
Should be rP19375 (first reference on svn blame on that file with the line that added std::map). So the actual mistake was to not move those includes as well from CinemaManager.h which now has unused includes, including the include whose absence Vladislav reported
So this should be it:
Index: source/graphics/CinemaManager.h =================================================================== --- source/graphics/CinemaManager.h (revision 19469) +++ source/graphics/CinemaManager.h (working copy) @@ -19,9 +19,6 @@ #ifndef INCLUDED_CINEMAMANAGER #define INCLUDED_CINEMAMANAGER -#include <list> -#include <map> - #include "lib/input.h" // InReaction - can't forward-declare enum #include "simulation2/helpers/CinemaPath.h" Index: source/simulation2/components/ICmpCinemaManager.h =================================================================== --- source/simulation2/components/ICmpCinemaManager.h (revision 19469) +++ source/simulation2/components/ICmpCinemaManager.h (working copy) @@ -18,6 +18,9 @@ #ifndef INCLUDED_ICMPCINEMAMANAGER #define INCLUDED_ICMPCINEMAMANAGER +#include <list> +#include <map> + #include "simulation2/helpers/CinemaPath.h" #include "simulation2/system/Interface.h"
Compiles when doing update-workspaces.sh --without-pch before make.
When only applying the CinemaManager.h without the ICmpCinemaManager.h patch, the build fails as reported.
Comment Actions
If it builds and someone else changes something that causes it to break blame is on someone else for not fixing that.
Should be rP19375
Ah, I guess I could have done some digging. Anyway, I've wasted enough time on discussing a roughly one line diff, so raising an issue it is.