Currently Online

Latest Posts

Topic: Travis codechecking misbehaviour

Nordfriese
Avatar
Topic Opener
Joined: 2017-01-17, 18:07
Posts: 1929
OS: Debian Testing
Version: Latest master
Ranking
One Elder of Players
Location: 0x55555d3a34c0
Posted at: 2020-01-30, 20:04

Can anybody make sense of this travis log?
https://travis-ci.org/widelands/widelands/jobs/643951401

Formatting finished.
++++git status -s
+++[[ -n M  src/editor/ui_menus/scenario_tool_field_owner_options_menu.cc ]]
+++echo 'Code not properly formatted. Please run: '\''./utils/fix_formatting.py'\'''
Code not properly formatted. Please run: './utils/fix_formatting.py'
+++echo 'Also, consider installing the githooks by running: '\''./install-githooks.sh'\'''
Also, consider installing the githooks by running: './install-githooks.sh'
+++git diff
+++exit 1
The command "./.travis.sh formatting" exited with 1.

I added git diff to travis.sh to see what exactly the issue corrected by clang-format is but it seems to result in an empty diff. Running clang-format locally on the offending file does not modify it at all...


Top Quote
hessenfarmer
Avatar
Joined: 2014-12-11, 23:16
Posts: 2646
Ranking
One Elder of Players
Location: Bavaria
Posted at: 2020-01-30, 21:30

problem is that our format script works differently on different machines. for this it was automated in former times on our bunnybot. This doesn't work on travis currently as travis can't write its changes back to github. I believe we need to find a way to do exactly this: writing the changes back to github and commit them.
until then you could only try to change the file arbitrarily and hope the next run on your local machine would be sufficient. BTW: with my last commit travis was happy with the branch but not with the merged master so it is really some kind of gambling. face-wink.png


Top Quote
Nordfriese
Avatar
Topic Opener
Joined: 2017-01-17, 18:07
Posts: 1929
OS: Debian Testing
Version: Latest master
Ranking
One Elder of Players
Location: 0x55555d3a34c0
Posted at: 2020-01-30, 21:49

Thanks for the explanation face-smile.png

I normally format the files automatically with the git hook on my linux, but ATM I only have my windows machine for sporadic fixes available.

Now I am really missing good old bunnybot... Perhaps we could hack together a script to let github run clang-format on PRs and master using this: https://help.github.com/en/actions/automating-your-workflow-with-github-actions/about-github-actions

What I still don´t understand though is why travis reports the file as changed while simultanously outputting that git diff is empty...


Top Quote
Nordfriese
Avatar
Topic Opener
Joined: 2017-01-17, 18:07
Posts: 1929
OS: Debian Testing
Version: Latest master
Ranking
One Elder of Players
Location: 0x55555d3a34c0
Posted at: 2020-01-30, 22:11

Yes, I think actions are the way to go. I´ll try to hack together a script to make github automatically run fix_formatting whenever a branch is pushed so we won´t always have to worry about the one platform-dependant extra whitespace.


Top Quote
Nordfriese
Avatar
Topic Opener
Joined: 2017-01-17, 18:07
Posts: 1929
OS: Debian Testing
Version: Latest master
Ranking
One Elder of Players
Location: 0x55555d3a34c0
Posted at: 2020-01-31, 10:29

I succeeded in letting GitHub automatically run clang-format whenever I push to my widelands fork.

Formatting your code

Now I "only" have to extend the script to commit and push the changes back to the branch.

The formatting takes about 4-5 minutes due to the need to set up python and clang-format, is this an acceptable delay?

Edited: 2020-01-31, 12:52

Top Quote
hessenfarmer
Avatar
Joined: 2014-12-11, 23:16
Posts: 2646
Ranking
One Elder of Players
Location: Bavaria
Posted at: 2020-01-31, 10:55

Nordfriese wrote:

I succeeded in letting GitHub automatically run clang-format whenever I push to my widelands fork.

Run fix_formatting.py

Now I "only" have to extend the script to commit and push the changes back to the branch.

The formatting takes about 4-5 minutes due to the need to set up python and clang-format, is this an acceptable delay?

It takes almost that long on my dev machine as well so this isn't too bad. We should only avoid pushing until it is finished. And we need tu pull before the next push. So this should probably only happen for Pull Requests instead of branches.


Top Quote
Nordfriese
Avatar
Topic Opener
Joined: 2017-01-17, 18:07
Posts: 1929
OS: Debian Testing
Version: Latest master
Ranking
One Elder of Players
Location: 0x55555d3a34c0
Posted at: 2020-01-31, 10:59

I think if you push while the formatting is running, a running job might be automatically cancelled. Still have to test whether this works.

And we need tu pull before the next push. So this should probably only happen for Pull Requests instead of branches.

Good point, will try to implement such a filter


Top Quote
Nordfriese
Avatar
Topic Opener
Joined: 2017-01-17, 18:07
Posts: 1929
OS: Debian Testing
Version: Latest master
Ranking
One Elder of Players
Location: 0x55555d3a34c0
Posted at: 2020-01-31, 17:07

Turns out that workflows are not automatically cancelled, so I added a check for remote updates in which the outdated workflow just exits with 1. There´s still a time window of a few seconds where this might be problematic, but it should then just result in a "branches have diverged" error and cancel in the "old" workflow while the "new" workflow succeeds.

Pull requests are more problematic.
Apparently (?) when you create or update a PR, the action is triggered on the base branch and not on the fork, and pushing between forks is forbidden for security reasons. Still need to figure out whether there´s a workaround...


Top Quote
stonerl
Avatar
Joined: 2018-07-30, 00:03
Posts: 327
Ranking
Tribe Member
Posted at: 2020-01-31, 17:52

I don't think that this is machine dependent. Here is the diff after running codechek:

@ -56,8 +56,9 @@ ScenarioToolFieldOwnerOptionsMenu::ScenarioToolFieldOwnerOptionsMenu(
        list_.add(
           (boost::format(_("Player %1$s (%2$s)")) % std::to_string(static_cast<int>(p)) % name)
              .str(),
-          p, g_gr->images().get(
-                Widelands::get_tribeinfo(eia().egbase().map().get_scenario_player_tribe(p)).icon),
+          p,
+          g_gr->images().get(
+             Widelands::get_tribeinfo(eia().egbase().map().get_scenario_player_tribe(p)).icon),
           sel == p, (boost::format(_("Claim fields for %s")) % name).str());
    }

It is dependent on the clang-format version. I guess travis runs a newer version than you do. Here is the one I run:

clang-format version 9.0.0 (tags/google/stable/2019-05-14)


Top Quote
Nordfriese
Avatar
Topic Opener
Joined: 2017-01-17, 18:07
Posts: 1929
OS: Debian Testing
Version: Latest master
Ranking
One Elder of Players
Location: 0x55555d3a34c0
Posted at: 2020-01-31, 18:10

Thanks for the diff face-smile.png

But my current machine doesn´t find a newer clang-format than clang-format version 3.8.0-2ubuntu4 (tags/RELEASE_380/final). So I think some automation would be nice anyway...

I´ve found a way to check in my script for whether the branch has an open pull request, so my workflow is now starting after every push to every branch, and it cancels immediately unless it´s the master branch or a branch with open PR.


Top Quote