Issue 575960043: Install python modules from build directory (Closed)
DescriptionInstall python modules from build directory
This solves issues when installing from a separate directory which was broken after ab7a344f68 ("Cleanup python/ build rules."). Also avoid __pycache__ in source directory from the test.
Patch Set 1 #
Total comments: 8
MessagesTotal messages: 5
https://codereview.appspot.com/575960043/diff/557650043/make/lilypond-vars.make File make/lilypond-vars.make (right): https://codereview.appspot.com/575960043/diff/557650043/make/lilypond-vars.ma... make/lilypond-vars.make:20: export PYTHONPATH:=$(top-build-dir)/python/$(outconfbase):$(PYTHONPATH) This will potentially make the out-www build fail. It's much better to load the source files directly, because they're always there and they're always up to date. https://codereview.appspot.com/575960043/diff/557650043/python/GNUmakefile File python/GNUmakefile (right): https://codereview.appspot.com/575960043/diff/557650043/python/GNUmakefile#ne... python/GNUmakefile:27: local-test: default if you do it like this, this should depend on $(outdir)/book_base_test.py and the rule should use $< . Otherwise, editing the test file doesn't redo the copy.
Sign in to reply to this message.
https://codereview.appspot.com/575960043/diff/557650043/make/lilypond-vars.make File make/lilypond-vars.make (right): https://codereview.appspot.com/575960043/diff/557650043/make/lilypond-vars.ma... make/lilypond-vars.make:20: export PYTHONPATH:=$(top-build-dir)/python/$(outconfbase):$(PYTHONPATH) On 2020/04/05 21:48:46, hanwenn wrote: > This will potentially make the out-www build fail. It's much better to load the > source files directly, because they're always there and they're always up to > date. I don't see why this should break: It's now back to the state it was before you changed it. But I can change it to out/ if you prefer. https://codereview.appspot.com/575960043/diff/557650043/python/GNUmakefile File python/GNUmakefile (right): https://codereview.appspot.com/575960043/diff/557650043/python/GNUmakefile#ne... python/GNUmakefile:27: local-test: default On 2020/04/05 21:48:46, hanwenn wrote: > if you do it like this, this should depend on $(outdir)/book_base_test.py and > the rule should use $< . Otherwise, editing the test file doesn't redo the copy. No, default depends on $(OUT_PY_MODULES). And we really need all modules because book_base_test.py includes other files.
Sign in to reply to this message.
https://codereview.appspot.com/575960043/diff/557650043/make/lilypond-vars.make File make/lilypond-vars.make (right): https://codereview.appspot.com/575960043/diff/557650043/make/lilypond-vars.ma... make/lilypond-vars.make:20: export PYTHONPATH:=$(top-build-dir)/python/$(outconfbase):$(PYTHONPATH) On 2020/04/05 21:54:40, hahnjo wrote: > On 2020/04/05 21:48:46, hanwenn wrote: > > This will potentially make the out-www build fail. It's much better to load > the > > source files directly, because they're always there and they're always up to > > date. > > I don't see why this should break: It's now back to the state it was before you > changed it. But I can change it to out/ if you prefer. Loading .py files from out/ directories is an antipattern in the lilypond build that makes working on the scripts a PITA, because the dependency tracking doesn't work, and you have to remember to rebuild them by hand. Please don't reintroduce this. https://codereview.appspot.com/575960043/diff/557650043/python/GNUmakefile File python/GNUmakefile (right): https://codereview.appspot.com/575960043/diff/557650043/python/GNUmakefile#ne... python/GNUmakefile:27: local-test: default On 2020/04/05 21:54:40, hahnjo wrote: > On 2020/04/05 21:48:46, hanwenn wrote: > > if you do it like this, this should depend on $(outdir)/book_base_test.py and > > the rule should use $< . Otherwise, editing the test file doesn't redo the > copy. > > No, default depends on $(OUT_PY_MODULES). And we really need all modules because > book_base_test.py includes other files. you're right. I do think that this is not the way to go to suppress the __pycache__ directory. If that is a concern, we should just pass -B to the commandline.
Sign in to reply to this message.
https://codereview.appspot.com/575960043/diff/557650043/make/lilypond-vars.make File make/lilypond-vars.make (right): https://codereview.appspot.com/575960043/diff/557650043/make/lilypond-vars.ma... make/lilypond-vars.make:20: export PYTHONPATH:=$(top-build-dir)/python/$(outconfbase):$(PYTHONPATH) On 2020/04/06 19:50:35, hanwenn wrote: > On 2020/04/05 21:54:40, hahnjo wrote: > > On 2020/04/05 21:48:46, hanwenn wrote: > > > This will potentially make the out-www build fail. It's much better to load > > the > > > source files directly, because they're always there and they're always up to > > > date. > > > > I don't see why this should break: It's now back to the state it was before > you > > changed it. But I can change it to out/ if you prefer. > > Loading .py files from out/ directories is an antipattern in the lilypond build > that makes working on the scripts a PITA, because the dependency tracking > doesn't work, and you have to remember to rebuild them by hand. Please don't > reintroduce this. I don't really get that argument. As far as I understand, your recent change ensures that python/ is built first to get the modules before they are needed. AFAICT this dependency works correctly, and if you touch a .py file in the source directory it is correctly copied over and byte-compiled. Maybe you can describe what's an "antipattern" about this? https://codereview.appspot.com/575960043/diff/557650043/python/GNUmakefile File python/GNUmakefile (right): https://codereview.appspot.com/575960043/diff/557650043/python/GNUmakefile#ne... python/GNUmakefile:27: local-test: default On 2020/04/06 19:50:35, hanwenn wrote: > On 2020/04/05 21:54:40, hahnjo wrote: > > On 2020/04/05 21:48:46, hanwenn wrote: > > > if you do it like this, this should depend on $(outdir)/book_base_test.py > and > > > the rule should use $< . Otherwise, editing the test file doesn't redo the > > copy. > > > > No, default depends on $(OUT_PY_MODULES). And we really need all modules > because > > book_base_test.py includes other files. > > you're right. I do think that this is not the way to go to suppress the > __pycache__ directory. If that is a concern, we should just pass -B to the > commandline. It doesn't surpress __pycache__, it just uses the already compiled modules in the build directory. This way there is exactly one location where the cache is.
Sign in to reply to this message.
On 2020/04/06 20:28:59, hahnjo wrote: > https://codereview.appspot.com/575960043/diff/557650043/make/lilypond-vars.make > File make/lilypond-vars.make (right): > > https://codereview.appspot.com/575960043/diff/557650043/make/lilypond-vars.ma... > make/lilypond-vars.make:20: export > PYTHONPATH:=$(top-build-dir)/python/$(outconfbase):$(PYTHONPATH) > On 2020/04/06 19:50:35, hanwenn wrote: > > On 2020/04/05 21:54:40, hahnjo wrote: > > > On 2020/04/05 21:48:46, hanwenn wrote: > > > > This will potentially make the out-www build fail. It's much better to > load > > > the > > > > source files directly, because they're always there and they're always up > to > > > > date. > > > > > > I don't see why this should break: It's now back to the state it was before > > you > > > changed it. But I can change it to out/ if you prefer. > > > > Loading .py files from out/ directories is an antipattern in the lilypond > build > > that makes working on the scripts a PITA, because the dependency tracking > > doesn't work, and you have to remember to rebuild them by hand. Please don't > > reintroduce this. > > I don't really get that argument. As far as I understand, your recent change > ensures that python/ is built first to get the modules before they are needed. > AFAICT this dependency works correctly, and if you touch a .py file in the > source directory it is correctly copied over and byte-compiled. Maybe you can > describe what's an "antipattern" about this? That assumes that you are building a complete build from the toplevel directory. When working on (say) the mf/ directory (which needs tweaks to the scripts under scripts/build/ ), one needs to do "make -C ../scripts/" by hand, because mf/GNUmakefile doesn't declare dependencies on scripts/build/ . Running scripts directly from source reduces confusion because effects take place immediately after saving the source file. > > > > if you do it like this, this should depend on $(outdir)/book_base_test.py > > and > > > > the rule should use $< . Otherwise, editing the test file doesn't redo the > > > copy. > > > > > > No, default depends on $(OUT_PY_MODULES). And we really need all modules > > because > > > book_base_test.py includes other files. > > > > you're right. I do think that this is not the way to go to suppress the > > __pycache__ directory. If that is a concern, we should just pass -B to the > > commandline. > > It doesn't surpress __pycache__, it just uses the already compiled modules in > the build directory. This way there is exactly one location where the cache is. Sure, but in practical day to day development, we'll be running python on source files directly, so I think we'll continue to see __pycache__ directories. It's probably simpler to put __pycache__ into .gitignore.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||
