Skip to content

Conversation

TheLortex
Copy link
Contributor

@TheLortex TheLortex commented Jul 7, 2023

In 5.1 programmers will be able to add other runtime events. This PR modifies olly trace so that it includes custom events of common types (unit, int, span).

Conditional compilation was used to make it work on 5.0.

@TheLortex TheLortex mentioned this pull request Jul 7, 2023
@kayceesrk
Copy link
Collaborator

How did you test this? With 5.1.0~alpha2, opam pin . gives me the following error building the dependent package core:

[ERROR] The compilation of core.v0.16.0 failed at "dune build -p core -j 7".

#=== ERROR while compiling core.v0.16.0 =======================================#
# context     2.1.3 | macos/arm64 | ocaml-base-compiler.5.1.0~alpha2 | https://opam.ocaml.org#7b30d632
# path        ~/.opam/5.1.0~alpha2/.opam-switch/build/core.v0.16.0
# command     ~/.opam/opam-init/hooks/sandbox.sh build dune build -p core -j 7
# exit-code   1
# env-file    ~/.opam/log/core-97555-cfb0d4.env
# output-file ~/.opam/log/core-97555-cfb0d4.out
### output ###
# should be applied to '()'; using '(struct end)' is deprecated.
# [...]
# File "_none_", line 1:
# Warning 73 [generative-application-expects-unit]: A generative functor
# should be applied to '()'; using '(struct end)' is deprecated.
# 
# File "_none_", line 1:
# Warning 73 [generative-application-expects-unit]: A generative functor
# should be applied to '()'; using '(struct end)' is deprecated.
# 
# File "_none_", line 1:
# Warning 73 [generative-application-expects-unit]: A generative functor
# should be applied to '()'; using '(struct end)' is deprecated.



<><> Error report <><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
+- The following actions failed
| - build core v0.16.0
+- 
- No changes have been performed

@Sudha247
Copy link
Member

This failure on arm64 and mac is odd -

Uncaught exception:
  
  (Invalid_argument
   "Float.iround_nearest_exn: argument (nan) is too small or NaN")

FWIW, I was seeing the same error on #16 as well, it wasn't introduced by this PR. I have access to an arm64 machine, will try to reproduce this locally.

@TheLortex
Copy link
Contributor Author

@kayceesrk I didn't test the 5.1 alpha so no idea why core is not building there.

My tests were done on a 5.0 compiler with the custom events patch: https://github.com/TheLortex/ocaml/tree/custom-events-5.0-rc1.

@Sudha247
Copy link
Member

@kayceesrk I don't see any errors on 5.1.0~beta1. Maybe we should activate a GH Actions job for 5.1.

@kayceesrk
Copy link
Collaborator

Maybe we should activate a GH Actions job for 5.1.

Sounds good to me.

@kayceesrk
Copy link
Collaborator

Is this PR good to be merged @TheLortex @Sudha247?

let init () = () in
let cleanup () = Trace.close trace_file in
olly ~runtime_begin ~runtime_end ~init ~lifecycle ~cleanup exec_args
olly ~extra ~runtime_begin ~runtime_end ~init ~lifecycle ~cleanup
Copy link
Member

Choose a reason for hiding this comment

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

Did not do a full review. Do we want to add custom events to json as well?

Copy link
Collaborator

@kayceesrk kayceesrk Aug 31, 2023

Choose a reason for hiding this comment

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

For completeness sake, we should. This can be done as a separate PR if need be, but an issue must be created before we merge this PR.

@Sudha247 Sudha247 merged commit 29db43a into tarides:main Sep 11, 2023
Sudha247 added a commit to Sudha247/opam-repository that referenced this pull request Sep 27, 2023
CHANGES:

* Custom events for json (tarides/runtime_events_tools#24, @Sudha247)
* Improvements to correct gc-stats (tarides/runtime_events_tools#19, @Sudha247)
* olly trace: ingest custom events starting from OCaml 5.1 (tarides/runtime_events_tools#17, @TheLortex)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

* Custom events for json (tarides/runtime_events_tools#24, @Sudha247)
* Improvements to correct gc-stats (tarides/runtime_events_tools#19, @Sudha247)
* olly trace: ingest custom events starting from OCaml 5.1 (tarides/runtime_events_tools#17, @TheLortex)
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