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

LcovMerger not properly declared as a tool by StandaloneTestStrategy #4033

Open
benjaminp opened this issue Nov 7, 2017 · 5 comments
Open

LcovMerger not properly declared as a tool by StandaloneTestStrategy #4033

benjaminp opened this issue Nov 7, 2017 · 5 comments
Labels
coverage not stale Issues or PRs that are inactive but not considered stale P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-Server Issues for serverside rules included with Bazel type: bug

Comments

@benjaminp
Copy link
Collaborator

If coverage is enabled, TestActionBuilder sticks the LcovMerger tool's FilesToRun into the test action's inputs. This suffices to build the LcovMerger executable and runfiles tree before running the test. However, since StandaloneTestStrategy does not include LcovMerger as a tool in the underlying test-running spawn, the sandbox fails to mount LcovMerger's runfiles tree into the sandbox execroot. This "works" now because the wrapper script for LcovMerger follows symlinks back into the real execroot. Given that such behavior isn't correct, a future stricter sandbox implementation could break coverage.
@damienmg
Copy link
Contributor

damienmg commented Nov 8, 2017

Routing through @lberki to triage.

@ulfjack ulfjack assigned iirina and unassigned lberki May 23, 2018
@ulfjack
Copy link
Contributor

ulfjack commented May 23, 2018

Depending on where and how fast we're going with coverage, we may not need to fix this.

bazel-io pushed a commit that referenced this issue May 28, 2018
@benjaminp
Treat runfiles middlemen as aggregating middlemen.
c868c47 
The goal is to, for an action execution, make only a single skyframe
edge to every required runfiles tree rather than an edge to every
runfiles artifact directly. We accomplish this by pulling the runfiles
artifacts' metadata for a particular runfiles tree through the
appropriate runfiles middlemen artifact in one batch. This CL fixes
#3217.

This change makes runfiles middlemen more similar to aggregating
middlemen. We however continue to treat runfiles middlemen slightly
differently than true aggregating middlemen. Namely, we do not expand
runfiles middlemen into the final action inputs. The reasons for this
are:

1. It can make latent bugs like
#4033 more severe.

2. The runfiles tree builder action's output MANIFEST contains
absolute paths, which interferes with remote execution if its metadata
is added to a cache hash.

3. In the sandbox, it causes the runfiles artifacts to be mounted at
their exec path locations in addition to within the runfiles
trees. This makes the sandbox less strict. (Perhaps tolerably?)

4. It saves a modicum of (transient) memory and time to not expand.

If the first two issues are fixed, it could be a nice simplification
to completely replace runfiles middlemen with aggregating middlemen.

Change-Id: I2d2148297f897af4c4c6dc055d87f8a6fad0061e
PiperOrigin-RevId: 198307109
@lberki lberki added team-Rules-Server Issues for serverside rules included with Bazel P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed category: rules > misc native labels Dec 3, 2018
@benjaminp benjaminp mentioned this issue May 15, 2019
Closed
@benjaminp benjaminp mentioned this issue Dec 9, 2019
Closed
@Toxicable Toxicable mentioned this issue Feb 3, 2020
Merged
4 tasks
@Toxicable
Copy link

Toxicable commented Feb 3, 2020 •
edited
Loading

We just came into this issue trying to implement bazel coverage in rules_nodejs. bazelbuild/rules_nodejs#1135
It dosen't look like this is goign to be fixed anytime soon.
I think we could work around it by moving the logic that was meant to be in the lcov_merger into the test runner itself.
Would this be the preferred work around?

@benjaminp benjaminp mentioned this issue Sep 17, 2020
Closed
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Treat runfiles middlemen as aggregating middlemen.
651453e 
    The goal is to, for an action execution, make only a single skyframe
    edge to every required runfiles tree rather than an edge to every
    runfiles artifact directly. We accomplish this by pulling the runfiles
    artifacts' metadata for a particular runfiles tree through the
    appropriate runfiles middlemen artifact in one batch. This CL fixes
    bazelbuild/bazel#3217.

    This change makes runfiles middlemen more similar to aggregating
    middlemen. We however continue to treat runfiles middlemen slightly
    differently than true aggregating middlemen. Namely, we do not expand
    runfiles middlemen into the final action inputs. The reasons for this
    are:

    1. It can make latent bugs like
    bazelbuild/bazel#4033 more severe.

    2. The runfiles tree builder action's output MANIFEST contains
    absolute paths, which interferes with remote execution if its metadata
    is added to a cache hash.

    3. In the sandbox, it causes the runfiles artifacts to be mounted at
    their exec path locations in addition to within the runfiles
    trees. This makes the sandbox less strict. (Perhaps tolerably?)

    4. It saves a modicum of (transient) memory and time to not expand.

    If the first two issues are fixed, it could be a nice simplification
    to completely replace runfiles middlemen with aggregating middlemen.

    Change-Id: I2d2148297f897af4c4c6dc055d87f8a6fad0061e
    PiperOrigin-RevId: 198307109
@sgowroji sgowroji added the stale Issues or PRs that are stale (no activity for 30 days) label Feb 17, 2023
@sgowroji
Copy link
Member

Hi there! We're doing a clean up of old issues and will be closing this one. Please reopen (or ping me to reopen) if you’d like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.

@sgowroji sgowroji closed this as not planned Won't fix, can't repro, duplicate, stale Feb 17, 2023
@fmeum
Copy link
Collaborator

fmeum commented Feb 17, 2023

@sgowroji still relevant

@cushon Making the lcov merger a native image would be a decent workaround for this problem and also help with remote execution.

@sgowroji sgowroji reopened this Feb 17, 2023
@sgowroji sgowroji added not stale Issues or PRs that are inactive but not considered stale and removed stale Issues or PRs that are stale (no activity for 30 days) labels Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
coverage not stale Issues or PRs that are inactive but not considered stale P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-Server Issues for serverside rules included with Bazel type: bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@benjaminp @iirina @lberki @damienmg @fmeum @ulfjack @Toxicable @sgowroji