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

added -threads and -vmthreads option information to the ppx context #1336

Merged
merged 7 commits into from
Sep 15, 2017

Conversation

camlspotter
Copy link
Contributor

Background

PPX preprocessors which search existing modules, such as ppx_overload, ppx_implicits and ppx_import, use Config.load_path as the search path. Config.load_path is calculated by Compmisc.init_path using Clflags.include_dirs, Clflags.use_threads and Clflags.use_vmthreads.

Currently only Clflags.include_dirs is passed from the host compiler command to PPX preprocessors: this makes them running with Config.load_path different from the one of the host compiler when -thread or -vmthread is given to the host compiler. As a result, PPX preprocessors cannot find thread related modules only by calling Compmisc.init.

The patch

This patch adds Clflags.use_threads and Clflags.use_vmthreads to the PPX context so that PPX preprocessors can configure itself with the same load path as the host compiler.

#* *
#* OCaml *
#* *
#* Peter Zotov *
Copy link
Contributor

Choose a reason for hiding this comment

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

This, and the copyright notice below, do not seem right.

Copy link
Contributor Author

@camlspotter camlspotter Sep 15, 2017

Choose a reason for hiding this comment

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

Ouch, a stuipid copy and paste.

I am going to remove it, or do you need a copy right notice of my name ?

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is:

  • If this Makefile was pilfered from somewhere else, you should preserve the original author attribution, and add your name and copyright if you made non-trivial modifications.
  • If this Makefile is your own work, then it should have your name and copyright (and no-one else's to begin with).

In both cases they should have the LGPL paragraph.

I'm suspecting something along these lines didn't happen correctly at a previous time, since this Makefile has an INRIA copyright yet a non-INRIA personal affiliation.

Copy link
Contributor

@alainfrisch alainfrisch Sep 15, 2017

Choose a reason for hiding this comment

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

There is no copyright header in the testsuite tests themselves. @mshinwell Do you know why we keep them in the testsuite Makefiles?

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not.

Copy link
Contributor

Choose a reason for hiding this comment

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

@damiendoligez @xavierleroy What is the official recommendation w.r.t. copyright headers in the testsuite? Currently they are kept in Makefiles but not in tests themselves. Should we get rid of them in Makefiles as well (or at least not require them for new Makefiles)?

Copy link
Member

Choose a reason for hiding this comment

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

No copyright headers in the test suite except for test infrastructure scripts and Makefiles. In general, the test directories themselves should not have any headers, even in Makefiles. I just haven't gotten around to remove them yet (and I won't need to thanks to ocamltest).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so @camlspotter : can you just remove the header?

@alainfrisch
Copy link
Contributor

This looks good to me, the PPX could make use of the knowledge about whether the code is compiled with -thread/-vmthread for other purposes.

What I don't immediately see is why this is required for load_path to be correct, since load_path itself (not just include_dirs) is also passed to the ppx in the context. But perhaps something within the ppx recomputes load_path from include_dirs and other flags?

@camlspotter
Copy link
Contributor Author

Ah now I understand. Config.load_path is transferred via PPX contexts, as you wrote, but Compmisc.init_path resets it. If PPX performs some typing using Compile.interface or Compile.implementation, it calls Compmisc.init_path and the issue happens.

Without this patch, I can avoid the issue by saving the value of Config.load_path before PPX calls Compmisc.init_path then pushing the value back after Compmisc.init_path. But it requires dirty workaround coding and I would like to avoid it if possible.

@alainfrisch
Copy link
Contributor

Ok, fair enough. I guess that one could arrange not to destroy load_path when running in the context of a PPX, but I'm not sure this would be more satisfactory than the proposed solution.

If nobody else objects to it, I'm tempted to merge (after sorting out what needs to be done with the Makefile copyright notice).

@shindere
Copy link
Contributor

shindere commented Sep 15, 2017 via email

@mshinwell
Copy link
Contributor

@alainfrisch I think you can just push to camlspotter's branch to remove the header and merge.

@mshinwell
Copy link
Contributor

I did the merge with trunk myself. I also removed the offending copyright header and removed some trailing whitespace. I will merge this to trunk now.

@mshinwell mshinwell merged commit 89355a4 into ocaml:trunk Sep 15, 2017
@camlspotter
Copy link
Contributor Author

Thanks!

@camlspotter camlspotter deleted the ppx_context_with_use_threads-trunk branch September 17, 2017 01:17
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants