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

[configurator] more flexible #define parsing #3739

Merged

Conversation

rgrinberg
Copy link
Member

Allow duplicate values for the same key as long as they are the same.

@rwmjones see if this fixes the issue for you

@rgrinberg rgrinberg requested a review from nojb August 23, 2020 03:57
Copy link
Collaborator

@nojb nojb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rwmjones
Copy link

I'm going to test this right now.

@rwmjones
Copy link

Yes this does appear to work here, thanks. I have pushed this preliminary commit to the Fedora branch of dune where we'll use it to build the packages that failed last time (https://bugzilla.redhat.com/show_bug.cgi?id=1870368#c17).

@rwmjones
Copy link

We don't seem to be quite there. There is another dune error which may be the same:

Error: Duplicate values for CAIRO_VERSION_MAJOR:
- 00000000001
- 01

Full log here: https://kojipkgs.fedoraproject.org//work/tasks/6856/50036856/build.log

@rwmjones
Copy link

I grabbed the test program from the log and compiled it with LTO:

$ gcc -c `rpm --eval '%{optflags}'` `rpm --eval '%{__global_ldflags}'` test.c `pkg-config cairo --cflags`
$ strings test.o  | grep BEGIN
BEGIN-0-01-END
BEGIN-1-016-END
BEGIN-1-00000000016-END
BEGIN-0-00000000001-END

Note again that what you're seeing here is a GCC intermediate form. When using LTO the compiler does only a preliminary parse stage. Actual compilation happens in the linker.

@rgrinberg
Copy link
Member Author

rgrinberg commented Aug 24, 2020 via email

@rwmjones
Copy link

I added a dummy main function to the end of the test function. Note we have to somehow have main depend on s0 and s1 otherwise they are optimized out.

main()
{
  return (int)&s0 + (int)&s1;
}

Compiling to a final program, this was the output:

$ strings a.out  | grep BEGIN
BEGIN-1-00000000016-END
BEGIN-0-00000000001-END

I still think this whole approach is suspect and studying what autoconf does is the way to go.

@rwmjones
Copy link

rwmjones commented Aug 24, 2020

Could you show the configure script in question?

See the log: https://kojipkgs.fedoraproject.org//work/tasks/6856/50036856/build.log

@rgrinberg
Copy link
Member Author

I still think this whole approach is suspect and studying what autoconf does is the way to go.

We'd happily accept a PR taking us in this direction.

@rwmjones
Copy link

It's unlikely to happen soon. Is there any workaround I can use to get this working so I can build cairo?

@rwmjones
Copy link

Are the CFLAGS embedded in dune when it is built, or do they come from the environment when dune is used?

@rgrinberg
Copy link
Member Author

rgrinberg commented Aug 24, 2020 via email

@rwmjones
Copy link

Is it possible to adjust these when building dune? Our ocamlc -config includes LTO flags, and that's how we want it to be because distros are generally moving to enabling LTO everywhere. We just want dune to use a slightly different set of flags when running these tests as a workaround.

@rgrinberg
Copy link
Member Author

I pushed another attempt at a workaround. See if that works.

@rgrinberg
Copy link
Member Author

Is it possible to adjust these when building dune? Our ocamlc -config includes LTO flags, and that's how we want it to be because distros are generally moving to enabling LTO everywhere. We just want dune to use a slightly different set of flags when running these tests as a workaround.

It's not possible, and I'm not sure it's the right way to solve this problem. Are there any other build tools where you configure the flags it passes at build time? We could just make dune respect CFLAGS and CPPFLAGS as well.

Let's see if @avsm has an opinion on this. I recall he wrestled with similar issues.

@rwmjones
Copy link

Yes that fixes ocaml-cairo (and didn't break ocaml-ssl), thanks. I'll add this patch to Fedora as well and continue rebuilding packages with it.

@rgrinberg
Copy link
Member Author

@nojb Could you give this another glance if you don't mind? The implementation is now different.

Copy link
Collaborator

@nojb nojb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, modulo the suggested simplification.

otherlibs/configurator/src/v1.ml Outdated Show resolved Hide resolved
otherlibs/configurator/src/v1.ml Outdated Show resolved Hide resolved
@rgrinberg rgrinberg force-pushed the configurator-allow-duplicate-values branch 2 times, most recently from f813f1f to 239e3ad Compare August 25, 2020 03:01
Allow duplicate values for the same key as long as they are interpreted
as the same value.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg rgrinberg force-pushed the configurator-allow-duplicate-values branch from 239e3ad to 73f03a6 Compare August 25, 2020 03:03
@rgrinberg rgrinberg merged commit ad2d7e4 into ocaml:master Aug 25, 2020
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Sep 3, 2020
…lugin, dune-private-libs and dune-glob (2.7.1)

CHANGES:

- configurator: More flexible probing of `#define`. We allow duplicate values in
  the object file, as long as they are the same after parsing. (ocaml/dune#3739, fixes
  ocaml/dune#3736, @rgrinberg)

- Record instrumentation backends in dune-package files. This makes it possible
  to use instrumentation backends defined in installed libraries (eg via OPAM).
  (ocaml/dune#3735, @nojb)

- Add missing `.aux` & `.glob` targets to coq rules (ocaml/dune#3721, fixes ocaml/dune#3437,
  @rgrinberg)

- Fix `dune-package` installation when META templates are present (ocaml/dune#3743, fixes
  ocaml/dune#3746, @rgrinberg)

- Resolve symlinks before running `$ git diff` (ocaml/dune#3750, fixes ocaml/dune#3740, @rgrinberg)

- Cram tests: when checking that all test directories contain a `run.t` file,
  skip empty directories. These can be left around by git. (ocaml/dune#3753, @emillon)
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Sep 5, 2020
…lugin, dune-private-libs and dune-glob (2.7.1)

CHANGES:

- configurator: More flexible probing of `#define`. We allow duplicate values in
  the object file, as long as they are the same after parsing. (ocaml/dune#3739, fixes
  ocaml/dune#3736, @rgrinberg)

- Record instrumentation backends in dune-package files. This makes it possible
  to use instrumentation backends defined in installed libraries (eg via OPAM).
  (ocaml/dune#3735, @nojb)

- Add missing `.aux` & `.glob` targets to coq rules (ocaml/dune#3721, fixes ocaml/dune#3437,
  @rgrinberg)

- Fix `dune-package` installation when META templates are present (ocaml/dune#3743, fixes
  ocaml/dune#3746, @rgrinberg)

- Resolve symlinks before running `$ git diff` (ocaml/dune#3750, fixes ocaml/dune#3740, @rgrinberg)

- Cram tests: when checking that all test directories contain a `run.t` file,
  skip empty directories. These can be left around by git. (ocaml/dune#3753, @emillon)
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

Successfully merging this pull request may close these issues.

3 participants