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

Fix install of symlinks with installopt but without installoptopt #932

Merged
merged 1 commit into from Nov 25, 2016

Conversation

Projects
None yet
3 participants
@whitequark
Contributor

whitequark commented Nov 24, 2016

Since the ocamlc -> ocamlc.byte symlink is not installed when ocamlopt exists, building ocamlopt, but not *.opt compilers, results in no ocamlc symlink being installed at all. Since ln -sf is already used everywhere, just make the code less clever, and let the symlink be overwritten when appropriate.

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark Nov 24, 2016

Contributor

(I suppose the reason no one hit this before is that the chief reason to have ocamlopt but not ocamlopt.opt is cross-compilation...)

Contributor

whitequark commented Nov 24, 2016

(I suppose the reason no one hit this before is that the chief reason to have ocamlopt but not ocamlopt.opt is cross-compilation...)

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 24, 2016

Member

I'm not fond of the fact that your solution implies that the order in which you run the install rules impacts the result. It will be fine if you just make install, but maybe during development people run more specific rules manually and now they have to be careful about that.

What about "make the test more clever"? It seems to me that the code previously had a kind of "if ocamlopt exists then make installopt" logic, on which the more clever linking was piggy-backed. We could just separate the two things, make installopt when ocamlopt exist, but install ocamlc.byte as ocamlc when ocamlc.opt does not exist. That seems simple (not "too clever for its own good"), and correct.

The specification I would expect is the following one:

  • install commands reason on the state of what has been built
  • they install the "short name" symlink exactly when they detect that the version they are installing is the best that is available

It is not followed today, but it does not sound hard to follow it. Or maybe this rule is too simplistic for cross-compilation scenarios?

Member

gasche commented Nov 24, 2016

I'm not fond of the fact that your solution implies that the order in which you run the install rules impacts the result. It will be fine if you just make install, but maybe during development people run more specific rules manually and now they have to be careful about that.

What about "make the test more clever"? It seems to me that the code previously had a kind of "if ocamlopt exists then make installopt" logic, on which the more clever linking was piggy-backed. We could just separate the two things, make installopt when ocamlopt exist, but install ocamlc.byte as ocamlc when ocamlc.opt does not exist. That seems simple (not "too clever for its own good"), and correct.

The specification I would expect is the following one:

  • install commands reason on the state of what has been built
  • they install the "short name" symlink exactly when they detect that the version they are installing is the best that is available

It is not followed today, but it does not sound hard to follow it. Or maybe this rule is too simplistic for cross-compilation scenarios?

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark Nov 25, 2016

Contributor

I'm not fond of the fact that your solution implies that the order in which you run the install rules impacts the result. It will be fine if you just make install, but maybe during development people run more specific rules manually and now they have to be careful about that.

Unless I'm missing something, this is not the case, because install implies installopt implies installoptopt, whereas installoptopt installs more specific symlinks than installopt than install.

That said, your solution does seem more elegant to me also, so I can do that.

Contributor

whitequark commented Nov 25, 2016

I'm not fond of the fact that your solution implies that the order in which you run the install rules impacts the result. It will be fine if you just make install, but maybe during development people run more specific rules manually and now they have to be careful about that.

Unless I'm missing something, this is not the case, because install implies installopt implies installoptopt, whereas installoptopt installs more specific symlinks than installopt than install.

That said, your solution does seem more elegant to me also, so I can do that.

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark Nov 25, 2016

Contributor

@gasche Done. Remarkably the patch even became simpler.

Contributor

whitequark commented Nov 25, 2016

@gasche Done. Remarkably the patch even became simpler.

Show outdated Hide outdated Makefile
ln -sf ocamlc.byte$(EXE) ocamlc$(EXE); \
ln -sf ocamlopt.byte$(EXE) ocamlopt$(EXE); \
ln -sf ocamllex.byte$(EXE) ocamllex$(EXE); \
fi

This comment has been minimized.

@dra27

dra27 Nov 25, 2016

Contributor

Indentation is wrong for the fi

@dra27

dra27 Nov 25, 2016

Contributor

Indentation is wrong for the fi

This comment has been minimized.

@whitequark

whitequark Nov 25, 2016

Contributor

Fixed

@whitequark

whitequark Nov 25, 2016

Contributor

Fixed

@@ -254,7 +254,7 @@ install:
cd $(INSTALL_BINDIR); \
ln -sf ocamlc.byte$(EXE) ocamlc$(EXE); \
ln -sf ocamllex.byte$(EXE) ocamllex$(EXE); \
fi
fi

This comment has been minimized.

@dra27

dra27 Nov 25, 2016

Contributor

FWIW The indentation of this one was correct, it's the three lines above which should be indented 😄

@dra27

dra27 Nov 25, 2016

Contributor

FWIW The indentation of this one was correct, it's the three lines above which should be indented 😄

This comment has been minimized.

@whitequark

whitequark Nov 25, 2016

Contributor

This makes no sense, this conditional is exactly the same as the one ending on line 282.

@whitequark

whitequark Nov 25, 2016

Contributor

This makes no sense, this conditional is exactly the same as the one ending on line 282.

This comment has been minimized.

@dra27

dra27 Nov 25, 2016

Contributor

Sorry - just looked at the raw file. GitHub (or my browser) is rendering the diff view badly - lines 253-256 are showing at the same indentation. Sorry for the noise...

@dra27

dra27 Nov 25, 2016

Contributor

Sorry - just looked at the raw file. GitHub (or my browser) is rendering the diff view badly - lines 253-256 are showing at the same indentation. Sorry for the noise...

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Nov 25, 2016

Contributor

@gasche - is a Changes entry needed for this, in your opinion?

Contributor

dra27 commented Nov 25, 2016

@gasche - is a Changes entry needed for this, in your opinion?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 25, 2016

Member

That's up to @whitequark preference, but my inclination would be "yes": this change fixes an user-observable issue -- so observable that the observers decided to fix the behavior. (It could go in "Bug fixes" or "Compiler distribution build system".)

Member

gasche commented Nov 25, 2016

That's up to @whitequark preference, but my inclination would be "yes": this change fixes an user-observable issue -- so observable that the observers decided to fix the behavior. (It could go in "Bug fixes" or "Compiler distribution build system".)

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark Nov 25, 2016

Contributor

@gasche I have no real preference beyond having the bug fixed but your reasoning is convincing.

Contributor

whitequark commented Nov 25, 2016

@gasche I have no real preference beyond having the bug fixed but your reasoning is convincing.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Nov 25, 2016

Contributor

All looks good, thanks!

Contributor

dra27 commented Nov 25, 2016

All looks good, thanks!

@dra27 dra27 merged commit 0670dc2 into ocaml:trunk Nov 25, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@whitequark whitequark deleted the whitequark:installopt branch Nov 25, 2016

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment