Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moved UI screenshots inside the repository using git-lfs #8392

Merged
merged 14 commits into from Aug 31, 2016
Merged

Moved UI screenshots inside the repository using git-lfs #8392

merged 14 commits into from Aug 31, 2016

Conversation

@mnapoli
Copy link
Contributor

mnapoli commented Jul 21, 2015

Fixes #7726

FYI I placed the screenshots into a new directory called expected-screenshots instead of the existing expected-ui-screenshots. The reason for that is because the existing dir is a submodule, and handling conflicts/branches with such a change is very very painful. Maybe after merging we could rename the directory if anyone prefers so.

TODO:

  • rebase
  • update to v0.5.3
  • update developer documentation
  • ask core contributors to install git-lfs
  • update to v0.5.4
  • green tests
  • merge matomo-org/travis-scripts#1 and update the travis-script submodule to point to master
  • remove the expected-ui-screenshots submodule
@mnapoli mnapoli force-pushed the git-lfs branch from 73a942c to a729399 Jul 24, 2015
mnapoli added a commit to matomo-org/developer-documentation that referenced this pull request Aug 3, 2015
@mnapoli

This comment has been minimized.

Copy link
Contributor Author

mnapoli commented Aug 3, 2015

Current status: even though I updated the screenshots, the Travis build uses a previous version. This is very weird because the screenshots are correct (up to date) both locally and on GitHub, but the screenshots being used in the Travis build are incorrect (not up to date).

One possibility is that git lfs is somehow downloading an old version of the screenshots… The reason for that is still unknown (is it a problem with the git extension, the GitHub git-lfs server, etc…).

@mattab

This comment has been minimized.

Copy link
Member

mattab commented Aug 10, 2015

Hey @mnapoli, was there any progress or maybe you found some information what was not working? It would be so great to use git-lfs soon!

@mnapoli

This comment has been minimized.

Copy link
Contributor Author

mnapoli commented Aug 10, 2015

@mattab I was waiting for a new release, but I'll try to make it work today and see if I can get around this buggy behavior.

mnapoli added a commit that referenced this pull request Aug 10, 2015
mnapoli added a commit that referenced this pull request Aug 10, 2015
@mnapoli

This comment has been minimized.

Copy link
Contributor Author

mnapoli commented Aug 10, 2015

I'm still trying to make the build work (git lfs installation issues now with the new version). I just discovered this other issue and we might need to wait for it to be fixed: git-lfs/git-lfs#564 (may be fixed in v0.6.0)

I'll try to have a green build (to have this PR working at least).

mnapoli added a commit that referenced this pull request Aug 10, 2015
[skip ci]
mnapoli added a commit that referenced this pull request Aug 10, 2015
@mnapoli

This comment has been minimized.

Copy link
Contributor Author

mnapoli commented Aug 10, 2015

I'm still stuck at the same place: Travis downloads old versions of some screenshots… I think it's a bug with git-lfs but it could be the client or the server… (maybe git-lfs/git-lfs#564)

I would give it another try when v0.6.0 is released.

@mattab

This comment has been minimized.

Copy link
Member

mattab commented Aug 14, 2015

it looks like they fixed git-lfs/git-lfs#564 - which maybe would unblock us.

@mnapoli do you know if maybe they have nightly builds or beta builds that we could test already?

@mnapoli

This comment has been minimized.

Copy link
Contributor Author

mnapoli commented Aug 14, 2015

@mattab had a look last time but it requires compiling the whole thing and there are no instructions IIRC. I can check again next week.

@mattab

This comment has been minimized.

Copy link
Member

mattab commented Aug 14, 2015

if we're lucky, compiling could be as easy as ./configure && make && make install ?

@mnapoli

This comment has been minimized.

Copy link
Contributor Author

mnapoli commented Aug 14, 2015

Nope, no Makefile, they use Go and they have custom script for e.g. Debian builds.

@mnapoli

This comment has been minimized.

Copy link
Contributor Author

mnapoli commented Aug 19, 2015

Tried with the current master (matomo-org/travis-scripts@bfb563a) but it seems images downloaded by git-lfs are not even correct images unfortunately. The build shows all image comparison failing (https://travis-ci.org/piwik/piwik/jobs/76290929) with:

Processed screenshot does not match expected for ActionsDataTable_column_sorted.png (image magick error: compare: improper image header `/home/travis/build/piwik/piwik/tests/lib/screenshot-testing/../../../tests/UI/./expected-screenshots/ActionsDataTable_column_sorted.png' @ error/png.c/ReadPNGImage/3246.)

Not looking good for now.

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Oct 27, 2015

Had a rough look at this and noticed there were Error trying to create local media directory in '/home/travis/build/piwik/piwik/.git/lfs/objects/d1/5f': mkdir /home/travis/build/piwik/piwik/.git/lfs/objects/d1: permission denied errors in the output of git lfs fetch and it said (262 of 262 files) 0 B / 21.95 MB.

I fixed this and it does not output any errors anymore but says (262 of 262 files) 19.87 MB / 21.95 MB so it still seems to not fetch all data for some reasons. Maybe this bug will be fixed in the next git-lfs version and would be just wait until there is a new version available

@mattab

This comment has been minimized.

Copy link
Member

mattab commented Oct 27, 2015

Maybe this bug will be fixed in the next git-lfs version and would be just wait until there is a new version available

Maybe we could check if it's a known bug and if not we could report it to git-lfs team?

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Oct 27, 2015

I did and there are some reports and I think there's one PR in review that might fix it but could be also totally unrelated. I'd say for now easiest would be maybe just to wait for a little

@mattab

This comment has been minimized.

Copy link
Member

mattab commented Nov 24, 2015

Moving to 3.0.0-b1 so we give it again another try

@sgiehl sgiehl force-pushed the git-lfs branch from f685094 to 4d56f16 Jul 18, 2016
sgiehl added a commit that referenced this pull request Jul 18, 2016
sgiehl added a commit that referenced this pull request Jul 18, 2016
sgiehl added a commit that referenced this pull request Jul 18, 2016
sgiehl added a commit that referenced this pull request Jul 18, 2016
@sgiehl sgiehl force-pushed the git-lfs branch to e7d69b4 Jul 18, 2016
@sgiehl

This comment has been minimized.

Copy link
Member

sgiehl commented Aug 30, 2016

Are we going to merge that into 2.x? If so I would merge it to master this weekend and then merge master to 3.x, so it's included there, as well.

@sgiehl sgiehl force-pushed the git-lfs branch from 7c61955 to 91bfc36 Aug 30, 2016
@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Aug 30, 2016

sounds good to me. FYI: I have merged master into 3.x just yesterday so it shouldn't be as painful :)

@sgiehl

This comment has been minimized.

Copy link
Member

sgiehl commented Aug 30, 2016

Ok. I'll schedule this for friday night or saturday.

@sgiehl sgiehl force-pushed the git-lfs branch from 91bfc36 to 8a78ad8 Aug 31, 2016
@sgiehl sgiehl merged commit d278d48 into master Aug 31, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Aug 31, 2016

FYI: When you merge master => 3.x-dev I'd recommend to directly use the screenshots from https://github.com/piwik/piwik-ui-tests/tree/3.0-m06

@sgiehl

This comment has been minimized.

Copy link
Member

sgiehl commented Aug 31, 2016

Already about to do that. Currently trying to resolve the merge conflicts...

@mnapoli

This comment has been minimized.

Copy link
Contributor Author

mnapoli commented Aug 31, 2016

\o/ congrats!

@mattab mattab added the Enhancement label Oct 2, 2016
mattab added a commit that referenced this pull request Nov 9, 2016
was introduced in d57f4a8 
but deprecated with git lfs #8392
mattab added a commit that referenced this pull request Nov 14, 2016
was introduced in d57f4a8 
but deprecated with git lfs #8392
@mattab mattab deleted the git-lfs branch Dec 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.