search

Starlink / ast

Starlink AST Library
GNU Lesser General Public License v3.0
12 stars 11 forks source link

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

Open brianv0 opened 5 years ago

brianv0 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Commit 9d1772478744 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 commented 5 years ago

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 commented 5 years ago

Done. Version 8.7.2

demitri commented 4 years ago

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.