Skip to content

Automatically add --no-rosegment flag#456

Open
itsjunetime wants to merge 5 commits into
flamegraph-rs:mainfrom
itsjunetime:automatically_add_no_rosegment_flag
Open

Automatically add --no-rosegment flag#456
itsjunetime wants to merge 5 commits into
flamegraph-rs:mainfrom
itsjunetime:automatically_add_no_rosegment_flag

Conversation

@itsjunetime

Copy link
Copy Markdown

I regularly run my projects with various different profiles and configurations, and regularly forget to include the --no-rosegment flag when I try to run perf/flamegraph stuff. This causes some annoyance and confusion when I get the flamegraph and it doesn't show me any useful information, so I figured it would be nice to try to detect when it's needed and include it automatically.

To do so, this PR:

  1. Checks the default cargo toolchain's rust version
  2. Gets rustflags from the RUSTFLAGS env var and the target's rustflags settings within cargo/config.toml using cargo config get (on nightly; if the user doesn't have the nightly toolchain installed, it just silently doesn't check the flags from that source)
  3. Goes through them and detects:
    a. If they're using gold, which doesn't require this flag (according to the README)
    b. If they are explicitly using a linker which requires it
    c. If they've already specified the --no-rosegment flag

If ((they're using rust >= 1.90 && not explicitly using gold) || using a linker that needs this flag) && not already specifying the no-rosegment flag, then we add it to the RUSTFLAGS env var for cargo compilation.

I've also added some tests since this has some potentially finnicky parsing behavior, and adding configuration to CI to run those tests.

@itsjunetime

Copy link
Copy Markdown
Author

Hmm, yeah I made some changes to CI and I guess I didn't understand how to use the rust-toolchain action correctly. Gotta poke around to fix it.

@djc djc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems to add a lot of specifically targeted complexity that I don't really feel confident reviewing, to the point that I'm not sure it's a good fit for the flamegraph tooling.

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.

2 participants