adding bindings to isl git repo

[Sven Verdoolaege]

Hi,

currently, the (C++ and Python) bindings are included
in the distribution, but not in the git repo.
The main reason for not adding them to git repo
is the general principle that generated files
should not be added to a repo.

Essentially, the reason is that there is no need to add generated files
because, well, they can be generated.
It's also a matter of duplication. The source information is already
in the repo.
Furthermore, some generated files may depend on the machine
on which they are generated so they would be different
for different users.

The bindings should be the same on all systems and
if for some reason they are not, we want to be able to detect that.
It's still duplication and technically anybody can regenerate the files,
but it is actually not that easy since it depends on the clang libraries,
while compiling isl itself just needs any decent C compiler.
Including the bindings also allows isl_test2 to be compiled
anywhere where a C++11 compiler is available and, recently,
many checks have been moved from (the C) isl_test to isl_test2,
while new checks are usually added to isl_test2.

The main reason, I'd like to add them, though, is that it makes
it easier to see the effect of changes on the bindings
since those resulting changes will also be part of the repo and
can be inspected just like the source changes.
In particular, this should make reviewing easier.

Any concerns or objections?

The changes should look something like the patch below
(+ the actual generated bindings).

skimo

diff --git a/Makefile.am b/Makefile.am
index a581a617d22..112845febea 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -21,8 +21,7 @@ noinst_PROGRAMS = isl_test isl_polyhedron_sample isl_pip \
isl_flow isl_flow_cmp isl_schedule_cmp
TESTS = isl_test codegen_test.sh pip_test.sh bound_test.sh isl_test_int \
flow_test.sh schedule_test.sh
-if HAVE_CPP_ISL_H
- CPP_H = include/isl/cpp.h include/isl/typed_cpp.h
+CPP_H = include/isl/cpp.h include/isl/typed_cpp.h
if HAVE_CXX11
noinst_PROGRAMS += isl_test2 isl_test_cpp
TESTS += isl_test2 isl_test_cpp isl_test_cpp_failed.sh
@@ -31,8 +30,6 @@ if HAVE_CXX17
noinst_PROGRAMS += isl_test_cpp17 isl_test_cpp17-checked
TESTS += isl_test_cpp17 isl_test_cpp17-checked
endif
-endif
-if HAVE_CLANG
if HAVE_CXX11
noinst_PROGRAMS += isl_test_cpp-checked isl_test_cpp-checked-conversion
TESTS += isl_test_cpp-checked isl_test_cpp-checked-conversion
@@ -42,7 +39,6 @@ if HAVE_PYTHON
isl_test_python.py: interface/isl.py libisl.la
endif
endif
-endif
TEST_EXTENSIONS = .py
AM_TESTS_ENVIRONMENT = \
export PYTHONPATH=interface; \
@@ -368,17 +364,14 @@ libdep_a_SOURCES = dep.c

if HAVE_CLANG
if HAVE_CXX11
-interface/isldlname.py: libisl.la
- $(AM_V_GEN) $(GREP) dlname $< | $(SED) -e 's/dlname/isl_dlname/' > $@
-interface/isl.py: interface/extract_interface$(BUILD_EXEEXT) libdep.a \
- python/isl.py.top interface/isldlname.py
- (cat interface/isldlname.py $(srcdir)/python/isl.py.top && \
- interface/extract_interface$(BUILD_EXEEXT) --language=python \
- $(includes) $(srcdir)/all.h) \
- > $@ || (rm $@ && false)
-
-include/isl/cpp.h: interface/extract_interface$(BUILD_EXEEXT) libdep.a \
- cpp/cpp.h.top cpp/cpp.h.bot
+$(srcdir)/interface/isl.py.core: interface/extract_interface$(BUILD_EXEEXT) \
+ libdep.a
+ interface/extract_interface$(BUILD_EXEEXT) --language=python \
+ $(includes) $(srcdir)/all.h \
+ > $@ || (rm $@ && false)
+
+$(srcdir)/include/isl/cpp.h: interface/extract_interface$(BUILD_EXEEXT) \
+ libdep.a cpp/cpp.h.top cpp/cpp.h.bot
$(MKDIR_P) "include/isl" && \
(cat $(srcdir)/cpp/cpp.h.top $(srcdir)/all.h && \
interface/extract_interface$(BUILD_EXEEXT) --language=cpp \
@@ -386,7 +379,8 @@ include/isl/cpp.h: interface/extract_interface$(BUILD_EXEEXT) libdep.a \
cat $(srcdir)/cpp/cpp.h.bot) \
> $@ || (rm $@ && false)

-include/isl/cpp-checked.h: interface/extract_interface$(BUILD_EXEEXT) libdep.a \
+$(srcdir)/include/isl/cpp-checked.h: \
+ interface/extract_interface$(BUILD_EXEEXT) libdep.a \
cpp/cpp-checked.h.top cpp/cpp-checked.h.bot
$(MKDIR_P) "include/isl" && \
(cat $(srcdir)/cpp/cpp-checked.h.top $(srcdir)/all.h && \
@@ -396,7 +390,7 @@ include/isl/cpp-checked.h: interface/extract_interface$(BUILD_EXEEXT) libdep.a \
cat $(srcdir)/cpp/cpp-checked.h.bot) \
> $@ || (rm $@ && false)

-include/isl/cpp-checked-conversion.h: \
+$(srcdir)/include/isl/cpp-checked-conversion.h: \
interface/extract_interface$(BUILD_EXEEXT) \
libdep.a \
cpp/cpp-checked-conversion.h.top \
@@ -409,7 +403,7 @@ include/isl/cpp-checked-conversion.h: \
cat $(srcdir)/cpp/cpp-checked-conversion.h.bot) \
> $@ || (rm $@ && false)

-include/isl/typed_cpp.h: interface/extract_interface$(BUILD_EXEEXT) \
+$(srcdir)/include/isl/typed_cpp.h: interface/extract_interface$(BUILD_EXEEXT) \
libdep.a cpp/typed_cpp.h.top cpp/typed_cpp.h.bot
$(MKDIR_P) "include/isl" && \
(cat $(srcdir)/cpp/typed_cpp.h.top && \
@@ -421,6 +415,14 @@ include/isl/typed_cpp.h: interface/extract_interface$(BUILD_EXEEXT) \
endif
endif

+interface/isldlname.py: libisl.la
+ $(AM_V_GEN) $(MKDIR_P) "interface" && \
+ $(GREP) dlname $< | $(SED) -e 's/dlname/isl_dlname/' > $@
+interface/isl.py: python/isl.py.top interface/isldlname.py \
+ $(srcdir)/interface/isl.py.core
+ cat interface/isldlname.py $(srcdir)/python/isl.py.top \
+ $(srcdir)/interface/isl.py.core > $@ || (rm $@ && false)
+
nodist_pkginclude_HEADERS = \
include/isl/stdint.h
pkginclude_HEADERS = \
@@ -488,19 +490,19 @@ pkginclude_HEADERS = \
if HAVE_CLANG
if HAVE_CXX11
CPP_INTERFACES = \
- include/isl/cpp.h \
- include/isl/cpp-checked.h \
- include/isl/cpp-checked-conversion.h \
- include/isl/typed_cpp.h
+ $(srcdir)/include/isl/cpp.h \
+ $(srcdir)/include/isl/cpp-checked.h \
+ $(srcdir)/include/isl/cpp-checked-conversion.h \
+ $(srcdir)/include/isl/typed_cpp.h
+ INTERFACES = $(CPP_INTERFACES) interface/isl.py
endif
endif
-BUILT_SOURCES = gitversion.h $(CPP_INTERFACES)
+BUILT_SOURCES = gitversion.h $(INTERFACES)
CLEANFILES = \
gitversion.h \
interface/isldlname.py \
interface/isl.py \
- interface/isl.pyc \
- $(CPP_INTERFACES)
+ interface/isl.pyc

DISTCLEANFILES = \
isl-uninstalled.sh \
@@ -678,6 +680,7 @@ EXTRA_DIST = \
imath/imath.h \
imath/imrat.c \
imath/imrat.h \
+ interface/isl.py.core \
all.h \
cpp \
python \
diff --git a/configure.ac b/configure.ac
index 15d1d3dd075..36313825f9d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -116,9 +116,6 @@ fi

AX_SUBMODULE(clang,system|no,no)
AM_CONDITIONAL(HAVE_CLANG, test $with_clang = system)
-AM_CONDITIONAL(HAVE_CPP_ISL_H,
- [(test $with_clang = system -a "x$HAVE_CXX11" = "x1") || \
- test -f $srcdir/include/isl/cpp.h])

AX_SET_WARNING_FLAGS

[Uday Reddy B]

You've described in the first two paras why such generated files shouldn't be added. The last para doesn't look like a great justification to go in the other direction. Perhaps there is another way to meet both objectives. If you want to see the impact of the changes on the bindings, can you add tests that use the Python bindings in the test suite so that the tests fail if they aren't updated to be in sync with the generated bindings? In that case, the changes in the commit to those test cases would show the impact on the bindings AFAIU. Btw, does isl have automated testing via Github CI for example?

-Uday

--

---
You received this message because you are subscribed to the Google Groups "isl Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to isl-developme...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/isl-development/20230318205422.GM9271MdfPADPa%40purples.home.

[Sven Verdoolaege]

On Tue, Mar 21, 2023 at 03:34:56PM +0530, Uday K Bondhugula wrote:
> You've described in the first two paras why such generated files shouldn't
> be added. The last para doesn't look like a great justification to go in
> the other direction. Perhaps there is another way to meet both objectives.
> If you want to see the impact of the changes on the bindings, can you add

I don't just want to see the *impact* of the changes.
I want to see the changes *themselves*.
If a commit message say something like "export FOO to the templated interface",
I want to see that FOO actually gets added to the templated interface
in the commit itself and that nothing else is changed to the interface(s).
I also want to be able to tell the difference between the interfaces
in two given commits by simply doing "git diff", without having
to build the two versions and save the generated bindings to some
separate location.

> tests that use the Python bindings in the test suite so that the tests fail
> if they aren't updated to be in sync with the generated bindings? In that

It's not practical to add tests for the entire interface,
especially the templated interface. There's just too many variants
of too many methods.
Besides, it would only show that something was added to an interface,
not that it was done in the given commit and it wouldn't say anything
about stuff that may get added to the interfaces by mistake.

> case, the changes in the commit to those test cases would show the impact
> on the bindings AFAIU. Btw, does isl have automated testing via Github CI
> for example?

Not for the public version.
Anyway, I want _everyone_ to be able to run the C++ tests,
not just those people who can (be bothered to) install the clang libraries.

skimo