Page MenuHomeWildfire Games
Differential D425

Update the xml validator tool
ClosedPublic
Actions

Authored by fatherbushido on May 4 2017, 9:43 AM.

Details

Reviewers
• leper
Commits
rP19660: Update the xml validator tool to handle mods. Reviewed by leper.
Trac Tickets
#3893
Summary

The tools introduced in #245 needs some updates as some rnc files are in mod and other in public.
The attached diff is not the thing to be commited as it's obviously ugly, it's just to open discussion and grab input.

Map rnc has been fixed in D391
Actor rnc seems needed an update? or those files are messed?

Test Plan

-

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

fatherbushido created this revision.May 4 2017, 9:43 AM
• leper added a subscriber: sanderd17.May 4 2017, 10:53 AM

About the actor files the breakage is most likely related to @sanderd17's extension for loading animation variants from files, so we might want to do that loading in here too (and skip those files themself).

source/tools/xmlvalidator/validate.pl
14 ↗(On Diff #1638)

This should walk the mod list in reverse (that is highest to lowest priority) order, and pick the first one it encounters.

21 ↗(On Diff #1638)

Same here, just picking a subpath of the above.

25 ↗(On Diff #1638)

This one might need some work.

Very rough pseudo-code:

for mod in mods # sorted by priority (increasing)
  find things in mod
  if ext == .DELTED
    remove everything that starts with that path (including filename)
  else
     add them

But since I already started with the slightly hacky (as in ignoring .DELETED, but good enough for most practical purposes) way one could just do

for mod in mods.reversed # guess what
  if file not yet added
    add it
fatherbushido added a comment.May 4 2017, 3:54 PM

Ah there was a ticket #3893

fatherbushido updated the Trac tickets for this revision.May 4 2017, 4:30 PM
Vulcan added a subscriber: Vulcan.May 4 2017, 7:38 PM

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/998/ for more details.

Harbormaster completed remote builds in B1437: Diff 1638.May 4 2017, 7:38 PM
fatherbushido added a comment.May 18 2017, 7:14 PM

Thanks for all the hints!
One should get the sorted mods list somewhere or hardcode it here?

• leper added a comment.May 18 2017, 9:48 PM

One should get the sorted mods list somewhere or hardcode it here?

I'd just hardcode it in some variable near the top, so that in case someone wants to change it they don't have to search a lot.

• leper requested changes to this revision.May 19 2017, 7:18 PM

(mostly so my review queue doesn't look that intimidating; if there are any questions I can help answering just ask)

This revision now requires changes to proceed.May 19 2017, 7:18 PM
fatherbushido updated this revision to Diff 2053.May 20 2017, 7:11 PM
fatherbushido edited edge metadata.
fatherbushido edited the summary of this revision. (Show Details)

Update

• leper requested changes to this revision.May 20 2017, 9:27 PM

Looking good!

source/tools/xmlvalidator/validate.pl
13 ↗(On Diff #2053)

I'd appreciate a comment outlining what we are doing, and why that is mostly ok (ignoring .DELETED, approximating the rest (or rather doing what that would result in otherwise)).

26 ↗(On Diff #2053)

Pointless newline.

41 ↗(On Diff #2053)

I guess we should check that all mods listed above actually exist in the right folder (ignoring things in the user mod folder (not the user mod)) first, then this existence check wouldn't be needed.

This revision now requires changes to proceed.May 20 2017, 9:27 PM
fatherbushido planned changes to this revision.May 20 2017, 9:30 PM
Vulcan added a comment.May 20 2017, 10:44 PM

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/1272/ for more details.

Harbormaster completed remote builds in B1826: Diff 2053.May 20 2017, 10:44 PM
fatherbushido mentioned this in D530: Update variants rnc/rng.May 21 2017, 5:27 PM
fatherbushido updated this revision to Diff 2153.May 23 2017, 6:54 PM
fatherbushido edited edge metadata.

Add a comment and remove the newline.

fatherbushido added inline comments.May 23 2017, 6:56 PM
source/tools/xmlvalidator/validate.pl
16 ↗(On Diff #2153)

I don't know if it's accurate and explicit enough.

26 ↗(On Diff #2053)

removed

41 ↗(On Diff #2053)

I will look at this. In fact I didn't really understand what you exactly mean ;-)

Vulcan added a comment.May 23 2017, 7:41 PM

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/1328/ for more details.

Vulcan added a comment.May 23 2017, 7:42 PM

Build is green

Executing section Default...
Executing section Source...
Executing section JS...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/21/ for more details.

Harbormaster completed remote builds in B1923: Diff 2153.May 23 2017, 7:42 PM
• leper accepted this revision.May 23 2017, 9:35 PM

Ack with that comment fix.

source/tools/xmlvalidator/validate.pl
16 ↗(On Diff #2153)

".DELETED files are not handled. This should still be good enough for most practical cases."

41 ↗(On Diff #2053)

This checks for every file in a mod that is listed (in @mods; actually @reversedmods) if that mod exists.

I would expect a failure or at least a warning if a mod that should be checked does not exist. So check for existence of "$vfsroot/$mod..

*looks at code again*

oh, disregard that; I might need some reading lessons

Why is that check needed?

This revision is now accepted and ready to land.May 23 2017, 9:35 PM
fatherbushido updated this revision to Diff 2165.May 24 2017, 2:27 PM

Fix the comment.

Harbormaster completed remote builds in B1935: Diff 2165.May 24 2017, 2:27 PM
fatherbushido added inline comments.May 24 2017, 2:32 PM
source/tools/xmlvalidator/validate.pl
41 ↗(On Diff #2053)

Here $vfspath is the folder where we want to find the files (like 'art/actors').
We try to find the files with the good ext ('.xml') in all those folders in each mod. So for example we have mod/art/actors but not public/art/actors.

Vulcan added a comment.May 24 2017, 3:40 PM

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/1336/ for more details.

Vulcan added a comment.May 24 2017, 3:41 PM

Build is green

Executing section Default...
Executing section Source...
Executing section JS...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/28/ for more details.

Harbormaster completed remote builds in B1935: Diff 2165.May 24 2017, 3:41 PM
Closed by commit rP19660: Update the xml validator tool to handle mods. Reviewed by leper. (authored by fatherbushido). · Explain WhyMay 26 2017, 2:44 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
ps/
trunk/
source/
tools/
xmlvalidator/
43 lines

Diff 2218

View Options

ps/trunk/source/tools/xmlvalidator/validate.pl

Loading...
Wildfire Games · Phabricator