ast.h subject to collision with python's ast.h in some build processes

[brianv0]

When compiling ast, by not placing the headers under a starlink directory, you are subject to collisions with python's ast.h.

For example, the following would fail because it will pick up python's ast.h first:

g++ -o src/MapSplit.os -c -std=c++14 -g -O3 -Wall -Wno-unused-function -fPIC \
  -Iinclude \
  -isystem /opt/conda/envs/default/include/python3.7m \
  -isystem /slac/opt/conda/envs/default/include \
   src/MapSplit.cc

While changing the order will succeed:

g++ -o src/MapSplit.os -c -std=c++14 -g -O3 -Wall -Wno-unused-function -fPIC \
  -Iinclude \
  -isystem /slac/opt/conda/envs/default/include \
  -isystem /opt/conda/envs/default/include/python3.7m \
   src/MapSplit.cc

[timj]

We can't easily remove ast.h from the installation location because it's been like that for 20 years and it's hard to know how much code would break if we moved it. I think what we can do is install ast.h in two places. The standard Starlink location these days would be star/ast.h and it would not hurt to do that. Then software could migrate over to the new location and eventually we could remove it. The starlink_pyast python package builds fine because it has its own copy of AST and presumably setuptools puts python last in the chain.

[brianv0]

Right. My recommendation would be to create a new minor version of ast which duplicates the location of the headers and a new major version which omits them. We could package both in conda-forge for the meantime.

If we were packaging starlink/ast under EPEL rules, we would have needed to resolve this issue before packaging, even if there is no current conflict, the filename is likely to be used by another project (https://www.google.com/search?q=inurl%3Aast.h+site%3Agithub.com&oq=inurl%3Aast.h+site%3Agithub.com
). I can't find any sort of guidelines the conda-forge community has on these issues, but I believe the guidance would be similar to avoid possible conflicts with any one of the other thousands of packages on conda-forge.

Since the conda-forge recipe didn't exist before and doesn't have a legacy of use, I have a slight preference towards moving ast*.h to starlink/. soon to make sure we avoid conflicts.

[timj]

Firstly, rules are annoying for packages that have existed for 20 years and long before some of these clashing files were in the wild (Starlink had a similar problem when ImageMagick suddenly started turning up on machines with a "convert" command). Secondly, it has to be star/ast.h to be at all consistent with all the other Starlink include files that turned up in the past 10 years. Thirdly, there is a lot of code in Starlink to change if we are requiring that ast.h is dropped, and there are users in the wild who will need to make the change as well. I'm not sure what sort of timeline you were hoping for cc/ @sfgraves

[timj]

It might be worth adding a configure option to not install the top level ast.h. Since conda-forge users haven't previously used AST then there would be no downside to using that option.

[dsberry]

Commit 9d17724 causes the header files to be installed into includedir/star. By default they are also installed in includedir (so legacy code is unaffected), but this can be prevented using the new "--without-topinclude" configure option. Note, in common with all other starlink libraries the error file ast_err.h is only installed into includedir.

[timj]

This is great. Thank you very much. Can you please make a formal release so we can grab the tar ball for conda-forge?

[dsberry]

Done. Version 8.7.2

[demitri]

I just came across this issue, which I would have missed if it were closed. This is potentially significant, and knowledge of it is useful to future-proof against a breaking change later. Can this information be added to a README at the top of this repo?

As a more general comment, a proper README for this repo would be welcome, if for nothing else than to "advertise" this library as a useful, active project. The lack of a README feel like a "code dump", which is certainly not the case here. I'll make this request a separate issue.