Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plplot buildfailure with transition from camlidl 1.05 -> 1.08 #18

Closed
opoplawski opened this issue May 22, 2020 · 7 comments
Closed

plplot buildfailure with transition from camlidl 1.05 -> 1.08 #18

opoplawski opened this issue May 22, 2020 · 7 comments

Comments

@opoplawski
Copy link

With the shift from ocaml-camlidl 1.05 -> 1.08 in Fedora rawhide we are getting a build failure in plplot:


[  4%] Generating plplot_core.idl, plplot_core.mli, plplot_core.ml, plplot_core_stubs.c, plplot_core.h
cd /builddir/build/BUILD/plplot-5.15.0/fedora/bindings/ocaml && /usr/bin/cmake -E copy /builddir/build/BUILD/plplot-5.15.0/bindings/ocaml/plplot_core.idl /builddir/build/BUILD/plplot-5.15.0/fedora/bindings/ocaml/plplot_core.idl
cd /builddir/build/BUILD/plplot-5.15.0/fedora/bindings/ocaml && /usr/bin/camlidl -I /builddir/build/BUILD/plplot-5.15.0/bindings/ocaml -header plplot_core.idl
File plplot_core.idl, line 369, column 13: Illegal character #

This line is:

RAW_ML(external plgriddata : float array -> float array -> float array -> float array -> float array -> plplot_grid_method_type -> float -> float array array = "ml_plgriddata_bytecode" "ml_plgriddata")

The macros involved are:

#define QUOTEME(x) #x
#define RAW_ML(x) quote(mlmli, QUOTEME(x));

Now, I know nothing about ocaml and camlidl so I'm hoping someone can clue us in on what has changed.

@rwmjones
Copy link

I am able to reproduce this easily:

$ camlidl -I . -header plplot_core.idl 
File plplot_core.idl, line 369, column 13: Illegal character #

However I don't understand how it can happen because when I pass the file through cpp myself there is no # character in any column except 1 (for line numbering).

@rwmjones
Copy link

OK I see why this happens. In camlidl the default definition for cpp is:

CPP=cpp -traditional

We don't change this in Fedora. Using the -traditional flag causes the macro expansion to happen differently.

Without -traditional:

quote(mlmli, "external plgriddata : float array -> float array -> float array -> float array -> float array -> plplot_grid_method_type -> float -> float array array = \"ml_plgriddata_bytecode\" \"ml_plgriddata\"");

With -traditional:

quote(mlmli, #external plgriddata : float array -> float array -> float array -> float array -> float array -> plplot_grid_method_type -> float -> float array array = "ml_plgriddata_bytecode" "ml_plgriddata");

We could easily change the Fedora build to remove this flag. However I presume the flag is added for a reason? If we leave the flag in, then plplot would have to change instead. Also I don't want to be different from what Debian & opam are doing as that will introduce unnecessary incompatibility that everyone will need to work around.

@xavierleroy
Copy link
Owner

Right, the change of behavior is due to this commit: 13f05c0.

I was nervous that running MIDL code (which is not C code) through a standard-conformant C preprocessor could cause errors, so I suggested the -traditional flag that makes the GNU C preprocessor much more lenient. I was not expecting that it would break legitimate use of C macros...

Should i just revert the change? Advice welcome.

@rwmjones
Copy link

The only examples of MSFT idl files I could easily find are from: https://docs.microsoft.com/en-us/windows/win32/com/anatomy-of-an-idl-file
The second example uses // comments. These are not removed when using -traditional but they are removed when using just cpp.

That would appear to be an argument to revert the change.

xavierleroy added a commit that referenced this issue May 22, 2020
This reverts commit 13f05c0.

As shown by issue #18, "-traditional" has its own problems and
breaks IDL code that worked before.
@xavierleroy
Copy link
Owner

These are compelling arguments. Reverted in commit e7593d0. Will make a new release ASAP.

@opoplawski
Copy link
Author

@xavierleroy @rwmjones thanks for handling this so quickly.

@xavierleroy
Copy link
Owner

This was fixed in release 1.09 in 2020, closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants