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

Use `BUILD_PATH_PREFIX_MAP` when compiling primitives #1856

Merged
merged 4 commits into from Jun 27, 2018

Conversation

Projects
None yet
4 participants
@xclerc
Copy link
Contributor

commented Jun 22, 2018

Both __LOC__ and __FILE__ may contain the path to the compiled
file, it hence seems logical to apply the substitution specified through the
BUILD_PATH_PREFIX_MAP environment variable when compiling
these primitives.

This PR is a one-liner; I guess the main question is: should we do something
if the path is not absolute?

@gasche

This comment has been minimized.

Copy link
Member

commented Jun 22, 2018

I see two questions:

  • This function assumes that an absolute path is given in argument. (In fact the implementation would work for some combinations of relative paths and BUILD_PATH_PREFIX_MAP values, but this is not considered in the specification.) So far, I have only needed to use it in code paths that were naturally guarded by a failed Filename.is_relative check or directly on Sys.getcwd (). I think that indeed you should only apply the function when the __FILE__ value is an absolute path; this can be done by adding a test at the time or application, or maybe adding a wrapper function Location.rewrite_path_if_absolute (do we have better naming ideas?).

  • Currently, Location.rewrite_absolute_path is only used on paths that are produced by the compilers, formed by Sys.getcwd () either alone or added before a user-provided relative path. The patch you propose is more general, it applies the transformation to a have path may have been provided (as an absolute path) by the user. I think that this is a sensible extension to make (especially if it comes with a concrete use-case where it improves reproducibility; for example, we have already discussed with @dbuenzli the fact that it is reasonable for build systems to always pass absolute paths to the compiler), but we should realize that this is a change in policy. (Is the long-term idea that we should rewrite all absolute paths, but we are doing the transformation lazily as problems are found? Or should we try to review now the absolute paths that are included in user-facing inputs?)

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2018

Regarding the first item, I pushed a change guarding the call
to Location.rewrite_absolute_path. Indeed, even if it would
work for some relative paths, it is probably worthwhile to apply
the principle of least surprise (with respect to the specification)
and only rewrite absolute paths.

Regarding the second item, I must admit do not follow your line
of reasoning: to me a path is relative or absolute independently
of its origin. Moreover, if a user sets BUILD_PATH_PREFIX_MAP
in order to have absolute paths rewritten and then passes absolute
paths to the compiler, she would presumably be extremely surprised
to see these absolute paths not rewritten.

@diml @rgrinberg as Dune developers, what is you take on passing
absolute paths to the compiler? (possibly only if the user activates
a "reproducible" mode)

@gasche

This comment has been minimized.

Copy link
Member

commented Jun 22, 2018

I can clarify my second point: when I proposed the BUILD_PATH_PREFIX_MAP feature, one of the things I said to convince people that it was innocuous is that the patch would only rewrite absolute paths built by the compiler, not absolute paths coming from the user. I am not saying it is a desirable property, personally I think that it is fine to rewrite user-provided (and I don't see a way to provide robust reproducibility without it), but I am just pointing out that the property wouldn't hold anymore after your patch -- maybe someone will look at the discussion at some point (possibly after a merge) and realize that it is a problem for them and say something.

@rgrinberg

This comment has been minimized.

Copy link
Member

commented Jun 23, 2018

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2018

@gasche thanks for the clarification! I would indeed be curious
to hear some feedback from these developers. I would argue
that to some extend it is easier for the user to anticipate the
effects of the substitution on paths she provides explicitly than
on paths built by the compiler.

@rgrinberg thanks for the feedback! I guess it is also OK on a
usability level; the log file will be slightly bigger, but that does
not seem to be an issue.

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2018

I don't think we need an entry in the change log for this PR.
@gasche is it ready to be merged?

@gasche

gasche approved these changes Jun 27, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

I approved the PR, but I would still feel a bit better if you included a Change entry. This is a change to the observable behavior of the compiler that users may want to be able to track down. Also, it advertises for the reproducible-compilation work, which is nice.

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2018

OK, done.

@gasche

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

(I'll let you take care of merging.)

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2018

Well, I would but I can't.

@gasche

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

... sigh, sorry. I I'll merge as soon as the CI returns.

@gasche gasche merged commit d880237 into ocaml:trunk Jun 27, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@shindere

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

This PR causes the output-complete-objtest to fail on OpenBSD because
ocamlc passes -fdebug-prefix-map to the C compiler although this option is
not supported. Would it be possible to provide a fix for this?

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

Sure, I will have a look. Can you check the value of
CC_HAS_DEBUG_PREFIX_MAP in config/Makefile?

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

Also, which compiler is used? (gcc or clang)

From what I read, clang supports the command-line
switch since at least 5.0 (and clang became the
default compiler with version 6.2).

(gcc supports the option since 4.3.)

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

In utils/ccomp.ml at line 96, it seems to me that debug_prefix_map is
called without checking that the option is acutally supported and the
function does not seem to check it either. Couldn't that be the
cause of this problem?

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

Yes, it is; I came to the same conclusion.

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

I will open a PR solving the issue shortly.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

@xclerc xclerc referenced this pull request Jun 29, 2018

Merged

Fix PR#1856 #1869

@xclerc xclerc referenced this pull request Nov 13, 2018

Closed

Reproducible builds #714

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.