Repository navigation
Conversation
Yhg1s
left a comment
Yhg1s
left a comment
There was a problem hiding this comment.
I'm not sure makesetup is the right place to do this. Its job is to generate the build rules for extension modules, and making it do more things means it's going to be harder to replace. Also, makesetup is run as part of configure, so if you create new directories (as part of development or as part of a build step) the resulting directory won't be installed.
Using find in makesetup is also a new requirement. find isn't as ubiquitous or standardised as bare sh, which makesetup has so far used.
How about instead of doing this in makesetup, changing the libinstall step in Makefile.pre.in to recurse into subdirectories when processing TESTSUBDIRS? It would require duplicating a bit of the logic to separate out the processing of LIBSUBDIRS from TESTSUBDIRS (we do not want to copy LIBSUBDIRS directories recursively, since they may contain test subdirs).
| echo "s%_MODOBJS_%$OBJS%" >>$sedf | ||
| echo "s%_MODLIBS_%$LIBS%" >>$sedf | ||
| # We used to have some test modules not installed, see gh-115421 | ||
| TESTSUBDIRS="$(cd Lib; find test -type d -not -path '*/__pycache__' | tr '\n' ' ')" |
There was a problem hiding this comment.
This is the first use of $() in this file, and I'm not sure about the portability of it (configure seems to have a fallback mechanism for $()). Given there's no nested backticks, probably safer to use those, like the rest of the file does.
-path '*/__pycache__' is usually written as -name __pycache__.
There was a problem hiding this comment.
Ok, your suggestion feels simplier! I will try to implement it soon, thanks for the idea!
|
Would it make sense to only copy directories which contain .py files? Maybe not. The Windows installer doesn't care and copy |
|
Well, I don't quite like the solution that I working on :( diff --git Makefile.pre.in Makefile.pre.in
index e37ae14326e..75e7ec6f68a 100644
--- Makefile.pre.in
+++ Makefile.pre.in
@@ -2239,8 +2239,8 @@ COMPILEALL_OPTS=-j0
TEST_MODULES=@TEST_MODULES@
-.PHONY: libinstall
-libinstall: all $(srcdir)/Modules/xxmodule.c
+.PHONY: commonlibinstall
+commonlibinstall:
@for i in $(SCRIPTDIR) $(LIBDEST); \
do \
if test ! -d $(DESTDIR)$$i; then \
@@ -2249,12 +2249,49 @@ libinstall: all $(srcdir)/Modules/xxmodule.c
else true; \
fi; \
done
- @if test "$(TEST_MODULES)" = yes; then \
- subdirs="$(LIBSUBDIRS) $(TESTSUBDIRS)"; \
- else \
- subdirs="$(LIBSUBDIRS)"; \
- fi; \
- for d in $$subdirs; \
+
+.PHONY: testlibinstall
+testlibinstall: $(srcdir)/Lib/test/* $(srcdir)/Lib/test/**/*
+ if test "$(TEST_MODULES)" = yes; then \
+ for entry in $^; \
+ do \
+ if test ! -d $$entry; then continue; else true; fi; \
+ if test `ls $$entry | wc -l` -lt 1; then continue; fi; \
+ case $$entry in \
+ **__pycache__**) \
+ continue; \
+ esac; \
+ libdest=$(LIBDEST)/$${entry#Lib/}; \
+ if test ! -d $(DESTDIR)$$libdest; then \
+ echo "Creating directory $$libdest"; \
+ $(INSTALL) -d -m $(DIRMODE) $(DESTDIR)$$libdest; \
+ else true; \
+ fi; \
+ for sub in $$entry/*; \
+ do \
+ if test -d $$sub; then continue; fi; \
+ case $$i in \
+ *CVS) ;; \
+ *.py[co]) ;; \
+ *.orig) ;; \
+ *~) ;; \
+ *) \
+ dest=$(DESTDIR)$(LIBDEST)/$${sub#Lib/}; \
+ if test -x $$sub; then \
+ echo $(INSTALL_SCRIPT) $$sub $$dest; \
+ $(INSTALL_SCRIPT) $$sub $$dest; \
+ else \
+ echo $(INSTALL_DATA) $$sub $$dest; \
+ $(INSTALL_DATA) $$sub $$dest; \
+ fi; \
+ esac; \
+ done; \
+ done; \
+ fi
+
+.PHONY: libinstall
+libinstall: all commonlibinstall testlibinstall $(srcdir)/Modules/xxmodule.c
+ for d in $(LIBSUBDIRS); \
do \
a=$(srcdir)/Lib/$$d; \
if test ! -d $$a; then continue; else true; fi; \
@@ -2275,12 +2312,7 @@ libinstall: all $(srcdir)/Modules/xxmodule.c
echo $(INSTALL_DATA) $$i $(LIBDEST); \
fi; \
done
- @if test "$(TEST_MODULES)" = yes; then \
- subdirs="$(LIBSUBDIRS) $(TESTSUBDIRS)"; \
- else \
- subdirs="$(LIBSUBDIRS)"; \
- fi; \
- for d in $$subdirs; \
+ for d in $(LIBSUBDIRS); \
do \
a=$(srcdir)/Lib/$$d; \
if test ! -d $$a; then continue; else true; fi; \This is the WIP, because it does not handle For my taste it is very complex, it repeats quite a lot of logic from Maybe we should test that all test folders are listed in |
|
Alternative PR: #115813 |
|
The alternative is merged. |
4 participants
It produces:
And here's what is installed:
Note that
__pycache__directories are created by compile job and they are not copied.This is the first time I touch
makesetup, so please, be very careful during the review :)