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.

Sign up for GitHub

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

Jump to bottom

src: fix build with --with-lttng #18945

Closed
wants to merge 1 commit into from
Closed

src: fix build with --with-lttng #18945

wants to merge 1 commit into from

Conversation

simark
Copy link

@simark simark commented Feb 22, 2018 •
edited
Loading

Building with --with-lttng hits a few issues.

  • There are missing parentheses in node_lttng_tp.h since

    src: lint node_lttng_tp.h
    f1d2792

    This patch adds them.

  • The arraysize function in node_lttng.cc is not found. I included
    node_internals.h and added the node namespace (node::arraysize).

  • The -llttng-ust flag is passed when building the static library libnode.a,
    but it's actually when linking a final executable that it's required.
    I see that node can be built as a shared library as well, so maybe the
    flag is relevant in that case. But in the case where the intermediate
    library is a static one, it doesn't work. I am not familiar with
    node's build system, so I did not fix this one, I am fine with
    changing the generated Makefile by hand for now. But if somebody
    wants to take a look, it's of course very appreciated :).

@simark
src: fix build with --with-lttng
d6c3254 
Building with --with-lttng hits a few issues.

- There are missing parentheses in node_lttng_tp.h since

  src: lint node_lttng_tp.h
  f1d2792

  This patch adds them.

- The arraysize function in node_lttng.cc is not found.  I included
  node_internals.h and added the node namespace (node::arraysize).

- The -llttng-ust is passed when building the static library libnode.a,
  but it's actually when linking a final executable that it's required.
  I see that node can be built as a shared library as well, so maybe the
  flag is relevant in that case.  But in the case where the intermediate
  library is a static one, it doesn't work.  I am not familiar with
  node's build system, so I did not fix this one, I am fine with
  changing the generated Makefile by hand for now.  But if somebody
  wants to take a look, it's of course very appreciated :).
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 22, 2018
@addaleax
Copy link
Member

@simark Is there some way in which the functionality of these things can be tested automatically? Node comes with support for a couple tracing systems, but I don’t think core contributors are actively making sure that they function. At least in my case, I’m not sure how exactly they are actually used in practice.

/cc @nodejs/diagnostics

@simark
Copy link
Author

simark commented Feb 22, 2018 •
edited
Loading

Is there a CI system that checks regression in the node code regularly? If so, you could make sure to have --with-lttng when building. On common distros, it only requires a few packages to be installed (e.g. liblttng-ust-dev and maybe others on Debian).

@addaleax
Copy link
Member

@simark Yes. 😄 And we do actually do that kind of thing for some build flags – but that would still only make sure that things compile, right?

@simark
Copy link
Author

simark commented Feb 22, 2018

@simark Yes. smile And we do actually do that kind of thing for some build flags – but that would still only make sure that things compile, right?

Exactly, but it's already better than nothing!

It would be easy enough to make a test that would look like:

  1. lttng create && lttng enable-event -u -a && lttng start
  2. Run node
  3. lttng stop
  4. Check that the command lttng view | grep <some event name we know should be there> works
  5. lttng destroy

@AndreasMadsen AndreasMadsen mentioned this pull request Feb 22, 2018
Closed
@AndreasMadsen
Copy link
Member

@simark The diagnostic WG have discussed reworking our tracing point implementation. Could you comment in nodejs/diagnostics#163 what your use case is?

The missing tests is a problem we have discussed and would like to solve in our refactor.

GlenTiki
GlenTiki approved these changes Feb 23, 2018
View reviewed changes
Copy link
Contributor

@GlenTiki GlenTiki left a comment

Choose a reason for hiding this comment

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

No one has touched these in nearly 2 years, they've been broken for a while.. I was about to open a PR to deprecate them. Good to see someone using it! 😆

@GlenTiki GlenTiki mentioned this pull request Feb 24, 2018
Closed
@ChALkeR ChALkeR mentioned this pull request Feb 24, 2018
Closed
2 tasks
@GlenTiki GlenTiki mentioned this pull request Feb 24, 2018
Closed
2 tasks
GlenTiki added a commit to GlenTiki/node that referenced this pull request Feb 25, 2018
@GlenTiki
src: Remove lttng support.
a170f7b 
This cleans up and removes lttng support completely. Recent discussion
on a PR to deprecate lttng suggested that we remove it completely
pending feedback from the TSC.

This should be considered a non breaking change, as a recent PR reveals
that compiling with this system has been broken for nearly two years.

Refs: nodejs#18971
Refs: nodejs#18975
Refs: nodejs#18945
GlenTiki added a commit that referenced this pull request Mar 1, 2018
@GlenTiki
src: Remove lttng support.
30fd3d2 
This cleans up and removes lttng support completely. Recent discussion
on a PR to deprecate lttng suggested that we remove it completely
pending feedback from the TSC.

This should be considered a non breaking change, as a recent PR reveals
that compiling with this system has been broken for nearly two years.

Refs: #18971
Refs: #18975
Refs: #18945

PR-URL: #18982
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
GlenTiki added a commit that referenced this pull request Mar 1, 2018
@GlenTiki
doc: add a deprecation message for removing lttng
740c426 
PR-URL: #18982
Refs: #18971
Refs: #18975
Refs: #18945
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member

BridgeAR commented Mar 1, 2018

If I am not mistaken this is now obsolete and should be closed? Ping @thekemkid

@GlenTiki
Copy link
Contributor

GlenTiki commented Mar 2, 2018

Yes, we merged the PR to remove lttng support yesterday. I'm closing this.

@GlenTiki GlenTiki closed this Mar 2, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@GlenTiki @MayaLekova
src: Remove lttng support.
fd24991 
This cleans up and removes lttng support completely. Recent discussion
on a PR to deprecate lttng suggested that we remove it completely
pending feedback from the TSC.

This should be considered a non breaking change, as a recent PR reveals
that compiling with this system has been broken for nearly two years.

Refs: nodejs#18971
Refs: nodejs#18975
Refs: nodejs#18945

PR-URL: nodejs#18982
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@GlenTiki @MayaLekova
doc: add a deprecation message for removing lttng
98660ee 
PR-URL: nodejs#18982
Refs: nodejs#18971
Refs: nodejs#18975
Refs: nodejs#18945
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GlenTiki GlenTiki GlenTiki approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@simark @addaleax @AndreasMadsen @BridgeAR @GlenTiki @nodejs-github-bot