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

Replace rake functions with python paver functions #2146

Closed
wants to merge 5 commits into from
Closed

Replace rake functions with python paver functions #2146

wants to merge 5 commits into from

Conversation

cybermnemosyne
Copy link
Contributor

New version to replace #2065 which replaced #1687

Of interest to @sarina, @singingwolfboy, @jarv, @e0d, @feanil, @wedaly, @jzoldak, @rocha, @mjg2203, @nedbat, @cpennington

I have deprecated the rake functions that paver replaces so far (more to come). The rake function will print a warning, wait 5 secs and then run the paver command. I have deleted the files and replaced them with file_deprecated.rake instead.

Have run through all of the functions that were output by rake -T

So far, now handles all of prereqs, assets, django and docs

Will be working on the remaining rake functions - so version 2 will have all rake functions replaced - grateful if people don't write any new ones?

I have cleaned up the process handling and also prerequisite caching from the initial version.

env = args[:env]
sh("paver compile_xmodule --system=#{system} --env=#{env}")
exit
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This set of tasks will be re-created many times. Was it left over from an early attempt at the deprecation?

In particular, for each call to deprecated("foo", "bar"), it'll create the assets, assets:preprocess, assets:coffee, etc tasks, as well as one task that actually uses the arguments "foo" and "bar"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - sorted out in next commit

" --recursive " + \
" --command=\'xmodule_assets common/static/xmodule\'" + \
" --wait " + \
" common/lib/xmodule"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python has implicit string continuation, so you can do this:

xmodule = (
    "watchmedo shell-command "
    " --patterns='*.js;*.coffee;*.sass;*.scss;*.css' "
    " --recursive "
    " --command=\'xmodule_assets common/static/xmodule\'"
    " --wait "
    " common/lib/xmodule"
)

Or reformat as you see fit to make it more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@singingwolfboy
Copy link
Contributor

This pull requests has conflicts with the latest version of master, and needs a rebase -- but once that's done, I think it's ready to merge (subject to approval of other reviewers). @dglance, can you do the rebase yourself and let us know when it's done, or would you like me to rebase your code on a new pull request, like I did with #2065?

@nedbat
Copy link
Contributor

nedbat commented Jan 15, 2014

I know rake deprecation was something we had requested be part of this pull request. This has turned out to be controversial, and requires a bit more preparation. I think we should leave the rake commands in place, but with clear messages in the rake files themselves that changes should be made in paver. Perhaps also produce a message when the command is run, but run it anyway.

For example, devops uses the rake commands as part of deploying, and needs time to adapt.

@cpennington
Copy link
Contributor

The rake deprecation in this PR doesn't remove the rake commands. It just makes them proxy through to paver, so it won't break any of devops' scripts.

Resubmit of rake replacement of assets, django, docs and prereqs
Deprecated rake functions issue a warning and then call paver
replacements
Changes on feedback from @cpennington and @singingwolfboy
Based on feedback from @singingwolfboy and @cpennington
@cybermnemosyne
Copy link
Contributor Author

Hi @nedbat - @cpennington is correct - the devops' scripts should be unaffected - but they should check obviously.

Could @singingwolfboy check that the rebase has worked as expected? If not - let me know or you can close this and do what you did before.

Not sure how you handle rebases I did:

git fetch upstream
git rebase upstream/master
git push -f origin rake2paver

return cmd


# This task takes arguments purely to pass them via dependencies to the preprocess task
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was true for the rake code. Is it still true for the python tasks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sort of - it isn't a particularly useful comment in any event

@cpennington
Copy link
Contributor

@dglance we've added a story for the LMS team for the next sprint (which starts on monday), so one or more of myself, @nedbat and @sarina will be looking at it for final review and merge then.

@singingwolfboy
Copy link
Contributor

I tried to deploy this branch on one of our sandbox servers, and it looks like it's not building CSS or JS for LMS. Oddly enough, it builds assets just fine for Studio. I don't know if this is the fault of your changes or of our sandbox deployment process. I do know that the sandbox deployment process uses rake, rather than paver, so it's going through the deprecation proxies. (I'm adding this comment simply so that the LMS team doesn't forget to check this branch on a sandbox server before merging it.)

Coffee command was changed in rakelib - updated to reflect this
Also removed comment that was ported over from rakelib
@cybermnemosyne
Copy link
Contributor Author

Re sandbox - the scripts should just do all the directories so there is no reason why it should work for cms and not lms - however I noticed that the way the coffeescript is specifying the directories has changed since I last looked and so I have updated it to do the same - this may fix the coffeescript issue.

Also removed carried over comments as per @cpennington's point above

@cybermnemosyne
Copy link
Contributor Author

Just to let you know that I have finished all of the other rake functions and am testing and writing docs at the moment (i.e. this would fully deprecate rake). I would still go ahead and finish this PR rather than take everything at once - the remaining rake functions involve more testing and things like translation that is difficult for me to completely test - so will need someone to check them over.

@shnayder
Copy link

@dglance The LMS team is going to be a bit slower than expected in doing final testing on this PR, because we're doing a big push to get platform internationalization done in time to launch several non-English classes next month, so I put everything else on hold. Once it's clear that we'll make that date, we'll get this merged--I expect in the first sprint in Feb. I'm sorry about the delay.

Completed rake to paver conversion
Added docs for paver
Done a first pass through documentation and rest of the system to
replace rake by paver
@cybermnemosyne
Copy link
Contributor Author

@shnayder @cpennington @singingwolfboy @wedaly @nedbat - as there is going to be this delay I have committed the rest of the conversion that includes a first pass at replacing rake and mentions of rake in the rest of the system. So this is a full deprecation - other than having the rake shim. It would be good if it could have some testing and feedback to fix things before the complete merge.

@@ -0,0 +1 @@
__author__ = 'dglance'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't usually put individual attributions in our files.

@wedaly wedaly mentioned this pull request Feb 7, 2014
@wedaly
Copy link
Contributor

wedaly commented Feb 24, 2014

Closing now that #2615 has been merged.

@wedaly wedaly closed this Feb 24, 2014
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Dec 22, 2017
* Correctly format date block dates.

(cherry picked from commit 22cb307)

* ECOM-3673 Update the course end date description text to remove the reference to certificate when course enrollment is in audit mode

(cherry picked from commit 649e8b1)

* Fixes for courseware date formatting/translation.

(cherry picked from commit 80bfa36)

 Conflicts:
	lms/djangoapps/courseware/date_summary.py

* Fix date summaries with Unicode format issues.

This issue would show up when date formatting strings are translated
with Unicode characters (like when testing with fake Esperanto
translations).

(cherry picked from commit c1a59cc)

* Converts the dates on the dashboard, sidebar navigation, and important course dates to user specified time zone.

(cherry picked from commit 0bf8fc4)

 Conflicts:
	common/djangoapps/util/date_utils.py
	common/lib/xmodule/xmodule/course_module.py
	common/lib/xmodule/xmodule/tests/test_course_metadata_utils.py
	lms/djangoapps/ccx/tests/test_models.py
	lms/djangoapps/courseware/views/index.py
	lms/djangoapps/student_account/views.py
	lms/templates/dashboard/_dashboard_course_listing.html
	openedx/core/djangoapps/content/course_overviews/models.py
	openedx/core/djangoapps/user_api/preferences/api.py
	openedx/core/djangoapps/user_api/preferences/tests/test_views.py

* Display JST for survey's answer datetime. openedx#2146

* Display JST for section's deadline and course start/end datetime. openedx#2146

* Display JST for face-to-face start/end datetime on Django Admin. openedx#2146

* Display JST for ORA2 settings. openedx#2146

* Add the validation of the schedule dates before 1900 openedx#2284 (openedx#2298)

* Implements command count_initial_registration_course openedx#2221 (openedx#2296)

* Add GaCourseEditorRole and GaLmsCourseStaffRole openedx#2150 (openedx#2306)

* Add GaCourseEditorRole and GaLmsCourseStaffRole openedx#2150

* Add UT for studio openedx#2150

* Fix GaCourseEditor has access to after the course closed openedx#2150

* GaLmsCourseStaff can use the same function as the course staff at the staff debug info openedx#2150

* Fix UT and Bok choy openedx#2150

* Remove extra line feed openedx#2150

* Fix comment openedx#2150

* Mod conditions openedx#2150

* Move judgment conditions to template openedx#2150

* Fix comment openedx#2150

* Remove unused code openedx#2150

* Mod conditions for GaCourseEditor but a global staff openedx#2150

* Revert parameter CourseDetails.update_from_json openedx#2150

* Mod check for staff acccess at LMS for GaCourseEditor openedx#2150

* Rename GaCourseEditorRole -> GaGlobalCourseCreatorRole openedx#2150

* Rename GA_COURSE_EDITOR_USER_INFO -> GA_GLOBAL_COURSE_CREATOR_USER_INFO openedx#2150

* Rename GA_ACCESS_CHECK_TYPE_COURSE_EDITOR -> GA_ACCESS_CHECK_TYPE_GLOBAL_COURSE_CREATOR openedx#2150

* Rename Ga_Course_Editor -> Ga_Global_Course_Creator openedx#2150

* Rename gacourseeditor -> gaglobalcoursecreator openedx#2150

* Rename GaCourseEditor -> GaGlobalCourseCreator openedx#2150

* Rename course_editor -> global_course_creator openedx#2150

* Mod hide staff debug info for GaGlobalCourseCreatorRole openedx#2150

* Fix UT for GaGlobalCourseCreator openedx#2150

* Rename UT openedx#2150

* Fix skipped UT openedx#2150

* Remove unused code openedx#2150

* Fix argument openedx#2150

* Remove unused code openedx#2150

* Fix ambiguous conditions openedx#2150

* Rename GaLmsCourseStaffRole -> GaCourseScorerRole openedx#2150

* Rename GA_LMS_COURSE_STAFF_USER_INFO -> GA_COURSE_SCORER_USER_INFO openedx#2150

* Ga_Lms_Course_Staff -> Ga_Course_Scorer openedx#2150

* Rename galmscoursestaff -> gacoursescorer openedx#2150

* Rename GaLmsCourseStaff -> GaCourseScorer openedx#2150

* Rename lms_course_staff -> course_scorer openedx#2150

* Fix translation openedx#2150

* Fix comment openedx#2150

*  Add personal info mask at resign openedx#2148 (openedx#2316)

* Add personal info mask at unenroll openedx#2148

* Fix Review.

* Add Unit Test.

* Add JST date fields near UTC date fields. openedx#2230 (openedx#2307)

* Add JST date fields near UTC date fields. openedx#2230

* Fix Review

* Fix Review 2.

* Add yml Files

* Add option of progress restriction that makes users need to pass prob… (openedx#2305)

* Add option of progress restriction that makes users need to pass problems. openedx#2147

* Skip Bok-Choy CoursewareProgressRestrictionTest temporarily
because this may cause an error of other test

* Fix review 1 openedx#2147

* Fix review 2 openedx#2147

* Fix to use different username and email from test_ga_register.py
so that no error occurs on test_register_and_activate. openedx#2147

* Fix review 3 openedx#2147

* Fix review 4 openedx#2147

* Fix review 5-1

* Fix review 5-2

* Fix review 5-3

* Fix review 5-4

* Fix review 5-5

* Fix review 5-6

* Fix review 5-7

* Fix review 5-8

* Fix review 5-9

* Fix review 5-10

* Fix review 5-12

* Fix review 5-14
revert 5-13

* Fix review 5-15

* Fix review 5-16

* Fix review 6-1 openedx#2147

* Fix review 6-2 openedx#2147

* Fix review 6-3 openedx#2147

* Fix UTC message in survey page. openedx#2340 (openedx#2345)

* Fix bug for manage_user_standing page. openedx#2347 (openedx#2348)

* Fix bug and add failed testcase for mask_resigned_user command. openedx#2362 (openedx#2365)

* Add confirm for account_disabled of manage_user_standing openedx#2366 (openedx#2367)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants