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

Fix 0.5 release branch + fixes to dist #2299

Merged

Conversation

benjaoming
Copy link
Contributor

@benjaoming benjaoming commented Sep 28, 2017

Summary

  • Fixes pre commit for python import sorter (relevant for all branches)
  • Cherry picks Android APK buildkit changes (which fixes broken Buildkite)
  • Updates setup.py to remove setup_requires (required for Debian builds, and YAGNI)
  • Removes stale BDD example code (YAGNI)
  • Should not ship BOTH with bundled deps as dependencies BIG WOOPS

Regarding the BIG WOOPS: This affects everything shipped so far. We have dependencies bundled AND specify them so they get pip installed, too.

pytest-runner allows us to do setup.py test such that it invokes pytest - however, we haven't fixed our sources to actually work without tox.ini, so only tox invocation works currently, anyways.

The problem with setup-requires is that it breaks default Debian building, and potentially other ways of building too (Debian's pybuild blocks requesting URLs for dependencies while building)

I've started building Debian packages in 0.5.0, which is why I'm lobbying for this change - but there's also a broken pre-commit hook, which is nicely dealt with.

I suppose we still intend to sync branches like release-v0.5.x into release-v0.6.x

@rtibbles rtibbles changed the base branch from l10n_release-v0.5.x to release-v0.5.x September 28, 2017 21:20
@codecov-io
Copy link

codecov-io commented Sep 28, 2017

Codecov Report

Merging #2299 into release-v0.5.x will decrease coverage by 2.31%.
The diff coverage is n/a.

Impacted file tree graph

@@                Coverage Diff                 @@
##           release-v0.5.x    #2299      +/-   ##
==================================================
- Coverage           79.17%   76.85%   -2.32%     
==================================================
  Files                 173      155      -18     
  Lines                7764     5760    -2004     
  Branches             1163      696     -467     
==================================================
- Hits                 6147     4427    -1720     
+ Misses               1459     1196     -263     
+ Partials              158      137      -21
Impacted Files Coverage Δ
kolibri/content/apps.py 0% <0%> (-100%) ⬇️
kolibri/content/utils/annotation.py 58.82% <0%> (-31.72%) ⬇️
kolibri/content/permissions.py 50% <0%> (-25%) ⬇️
kolibri/plugins/setup_wizard/middleware.py 75% <0%> (-11.96%) ⬇️
kolibri/content/serializers.py 81.81% <0%> (-9.3%) ⬇️
...libri/content/management/commands/exportcontent.py 71.11% <0%> (-9.25%) ⬇️
kolibri/utils/version.py 70.31% <0%> (-7.98%) ⬇️
kolibri/plugins/coach/serializers.py 65.78% <0%> (-6.78%) ⬇️
kolibri/content/api.py 51.91% <0%> (-6.43%) ⬇️
kolibri/core/webpack/hooks.py 63.42% <0%> (-4.9%) ⬇️
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1d906e...1a44655. Read the comment docs.

@benjaoming benjaoming changed the title Rm pytest-runner + fix pre-commit hook [WIP] rm pytest-runner + fix pre-commit hook Sep 28, 2017
@benjaoming
Copy link
Contributor Author

Unexpected bumps while building the Debian package, not at all intentional changes here - or now they are :)

@indirectlylit
Copy link
Contributor

It might be easier to just target this to develop, 0.7 - the initial public release.

I don't believe there's a need to make old releases available as debian packages?

@indirectlylit
Copy link
Contributor

actually - did we need deb for 0.6 @jamalex ?

@benjaoming
Copy link
Contributor Author

benjaoming commented Sep 30, 2017

@indirectlylit

If the 0.5 branch is still relevant to someone and has to be supported, I would merge this because of the fixes stated in the description.

Otherwise someone would have to fix it at a later time, anyways.

@benjaoming benjaoming changed the title [WIP] rm pytest-runner + fix pre-commit hook rm pytest-runner + fix pre-commit hook Sep 30, 2017
@benjaoming benjaoming changed the title rm pytest-runner + fix pre-commit hook Fix 0.5 release branch + fixes to dist Sep 30, 2017
@benjaoming
Copy link
Contributor Author

This is ready now, please see the above description.

@indirectlylit
Copy link
Contributor

Would you mind providing some notes on how to test this? We can give it a go and get it merged in.

If the 0.5 branch is still relevant to someone and has to be supported, I would merge this because of the fixes stated in the description.

It's not harmful to merge in, but we don't have any plans to release another version of 0.5 so this won't impact any users.

(This PR has conflicts with 0.6 and develop, which is why I was suggesting it would be easier to skip 0.5)

@benjaoming
Copy link
Contributor Author

(This PR has conflicts with 0.6 and develop, which is why I was suggesting it would be easier to skip 0.5)

Conflicts you say? It's confusing that release-v0.5.x isn't mergeable into release-v0.6.x - isn't that breaking some assumptions?

@benjaoming
Copy link
Contributor Author

Would you mind providing some notes on how to test this? We can give it a go and get it merged in.

Hmm, not sure how to test it as such - it's more I think a code review thing.

Maybe if it's verified that the artifacts from Buildkite are okay, then it's easy to say that the critical setup.py changes are okay? .deb builds are okay!

I can get this merged into the other branches after it lands in 0.5.0 - btw that's our current 'stable' release which is why I built the .debs for it, don't know if I ever mentioned that!! :)

@indirectlylit
Copy link
Contributor

It's confusing that release-v0.5.x isn't mergeable into release-v0.6.x - isn't that breaking some assumptions?

nope - 0.6 is fully up-to-date with the current state of 0.5.

There are just a couple minor conflicts with this PR, particularly with this line: https://github.com/learningequality/kolibri/blob/release-v0.6.x/Makefile#L68

@benjaoming
Copy link
Contributor Author

I dare merge this now. I've added a changelog entry as an unreleased '0.5.1dev'. The rationale would be the following:

  • Something serious enough was fixed in the 0.5.x series, so that should be released
  • Merging this branch into the other branches will give a conflict that I need to solve for the changelog. This is welcomed, as the 0.5.x changelog has to be inserted below other changelogs.

Once 0.5.1 is released, we will have to sync branches again, and the changelog will have a version bump - but this part shouldn't give any merge conflicts, as the 0.5.1dev entry is already there.

@benjaoming
Copy link
Contributor Author

Have tested the .pex and it works fine. Looking at file sizes, it doesn't seem like we have any problems with missing dependencies, neither.. all sizes remain intact.

The Android APK is now 38 MB (before: 21 MB), but guessing the build is very different now @christianmemije ?

@benjaoming benjaoming merged commit 7c6e50f into learningequality:release-v0.5.x Oct 6, 2017
@benjaoming benjaoming deleted the rm-pytest-runner branch October 6, 2017 16:32
@indirectlylit indirectlylit added this to the 0.5.1 milestone Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants