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

Improve handling of platforms that do not have gprof profiling support #933

Merged
merged 1 commit into from Nov 25, 2016

Conversation

Projects
None yet
3 participants
@shindere
Contributor

shindere commented Nov 24, 2016

Two questions:

  1. Is a fatal error okay when ocamlopt -p is used whereas profiling with
    gprof is not supported, or would a warning be better?

(I am personnally in favour of a fatal error but if people think this may
break existing code it can be a warning, too.)

  1. Where / how to best modify the Changes file for such a PR?
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 24, 2016

Member

Re. Changes: either "Compiler user-interface and warning" or "Code generation and optimization" would seem fine to me.

Member

gasche commented Nov 24, 2016

Re. Changes: either "Compiler user-interface and warning" or "Code generation and optimization" would seem fine to me.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Nov 25, 2016

Contributor
Contributor

shindere commented Nov 25, 2016

Improve handling of platforms that do not have gprof profiling support
On such platforms, ``make install'' was
installing dummy profiling
libraries and ocamlopt's -p option was silently ignored.

This commit modifies these two behaviours:

1. ``make install'' installs libraries with profiling support only when
this makes sense.

2. On platforms that do not support profiling with gprof, the -p option of
ocamlopt produces an error message.

In addition, this commit modifies the values of the PROFILING make
variable. Before the commit it was either prof or noprof. After the
commit it is either true or false.

In the asmrun directory, the call to ranlib for libasmrunp.a has also been
removed from the install target because this command is already invoked
in the rule that builds libasmrunp.a.

ocamlc/ocamlopt -config now prints the state of profiling support

@damiendoligez damiendoligez merged commit 8adfe15 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

gasche added a commit that referenced this pull request Nov 25, 2016

Revert "Improve handling of platforms that do not have gprof profilin…
…g support (#933)"

This reverts commit 8adfe15.

This is a temporary revert caused by Continuous Integration
failure. We'll investigate the issue and merge again when it is fixed.
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 25, 2016

Member

The INRIA CI is failing on this change (at least: trunk-cygwin-32, trunk-zsystems-64). I pushed the current trunk into precheck to get full reports, and then temporarily reverted the change.

Member

gasche commented Nov 25, 2016

The INRIA CI is failing on this change (at least: trunk-cygwin-32, trunk-zsystems-64). I pushed the current trunk into precheck to get full reports, and then temporarily reverted the change.

shindere added a commit that referenced this pull request Nov 25, 2016

Improve handling of platforms that do not have gprof profiling support (
#933)

This commit modifies these two behaviours:

1. ``make install'' installs libraries with profiling support only when
this makes sense.

2. On platforms that do not support profiling with gprof, the -p option of
ocamlopt produces an error message.
On such platforms, ``make install'' was installing dummy profiling
libraries and ocamlopt's -p option was silently ignored.

In addition, this commit modifies the values of the PROFILING make
variable. Before the commit it was either prof or noprof. After the
commit it is either true or false.

In the asmrun directory, the call to ranlib for libasmrunp.a has also been
removed from the install target because this command is already invoked
in the rule that builds libasmrunp.a.

ocamlc/ocamlopt -config now prints the state of profiling support
@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Nov 25, 2016

Contributor
Contributor

shindere commented Nov 25, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 25, 2016

Member

Ok, thanks. There was no issue with the failing CI of course, but I like to revert in a systematic way when that happens because it gives more time to people to relax and think about the fix -- although obviously that wasn't really necessary in this specific case, you solved the issue very quickly.

Member

gasche commented Nov 25, 2016

Ok, thanks. There was no issue with the failing CI of course, but I like to revert in a systematic way when that happens because it gives more time to people to relax and think about the fix -- although obviously that wasn't really necessary in this specific case, you solved the issue very quickly.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Nov 25, 2016

Contributor
Contributor

shindere commented Nov 25, 2016

@shindere shindere deleted the shindere:profiling-support branch Nov 26, 2016

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

Improve handling of platforms that do not have gprof profiling support (
#933)

This commit modifies these two behaviours:

1. ``make install'' installs libraries with profiling support only when
this makes sense.

2. On platforms that do not support profiling with gprof, the -p option of
ocamlopt produces an error message.
On such platforms, ``make install'' was installing dummy profiling
libraries and ocamlopt's -p option was silently ignored.

In addition, this commit modifies the values of the PROFILING make
variable. Before the commit it was either prof or noprof. After the
commit it is either true or false.

In the asmrun directory, the call to ranlib for libasmrunp.a has also been
removed from the install target because this command is already invoked
in the rule that builds libasmrunp.a.

ocamlc/ocamlopt -config now prints the state of profiling support

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

Revert "Improve handling of platforms that do not have gprof profilin…
…g support (#933)"

This reverts commit 8adfe15.

This is a temporary revert caused by Continuous Integration
failure. We'll investigate the issue and merge again when it is fixed.

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

Improve handling of platforms that do not have gprof profiling support (
#933)

This commit modifies these two behaviours:

1. ``make install'' installs libraries with profiling support only when
this makes sense.

2. On platforms that do not support profiling with gprof, the -p option of
ocamlopt produces an error message.
On such platforms, ``make install'' was installing dummy profiling
libraries and ocamlopt's -p option was silently ignored.

In addition, this commit modifies the values of the PROFILING make
variable. Before the commit it was either prof or noprof. After the
commit it is either true or false.

In the asmrun directory, the call to ranlib for libasmrunp.a has also been
removed from the install target because this command is already invoked
in the rule that builds libasmrunp.a.

ocamlc/ocamlopt -config now prints the state of profiling support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment