x test error_index_generator triggers 'nested groups are not supported by GHA' check #113798

Activity

1. oli-obk

   added

   T-bootstrapRelevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

   C-bugCategory: This is a bug.Category: This is a bug.

   on Jul 17, 2023
2. oli-obk

   assigned

   oli-obk

   and

   jyn514

   on Jul 17, 2023
3. rustbot

   added

   needs-triageThis issue may need triage. Remove when done. See docs forge.rust-lang.org/release/issue-triagingThis issue may need triage. Remove when done. See docs forge.rust-lang.org/release/issue-triaging

   on Jul 17, 2023
4. jyn514 commented on Jul 17, 2023

   

   jyn514

   on Jul 17, 2023

   Member

   More actions

   in general the fix for this is usually to add let rustdoc = builder.rustdoc_cmd before the builder.msg line so that they don't overlap
5. jyn514 commented on Jul 17, 2023

   

   jyn514

   on Jul 17, 2023

   Last edited by jyn514

   Member

   More actions

   we might want to consider changing DryRun::SelfCheck to bypass the type-based cache so it catches ordering bugs like this

   cc @Mark-Simulacrum
6. jyn514

   added this to the 1.73.0 milestone on Jul 17, 2023
7. jyn514

   added

   regression-from-stable-to-nightlyPerformance or correctness regression from stable to nightly.Performance or correctness regression from stable to nightly.

   and removed

   needs-triageThis issue may need triage. Remove when done. See docs forge.rust-lang.org/release/issue-triagingThis issue may need triage. Remove when done. See docs forge.rust-lang.org/release/issue-triaging

   on Jul 17, 2023
8. rustbot

   added

   I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}Issue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}

   on Jul 17, 2023
9. Mark-Simulacrum commented on Jul 17, 2023

   

   Mark-Simulacrum

   on Jul 17, 2023

   Member

   More actions

   I don't understand how dry run is relevant here, isn't this a problem in all cases? Dry run and real should have roughly the same ensure graph to the extent possible and our tests rely on that...

   It also seems like based on the linked PR here this assertion is always guaranteed to show up at runtime pretty much constantly anytime someone accidentally adds additional nesting with a group? I could see that the right fix is to track the group and end + restart it so that there's no nesting and instead a linear non-nested list of groups...

   👍React with 👍1Reacted by Jakub Beránek
10. jyn514 commented on Jul 17, 2023

    

    jyn514

    on Jul 17, 2023

    Last edited by jyn514

    Member

    More actions

    dry-run and real have the same problems, yes. my idea was to do extra checks in dry-run so that x test without arguments would have caught this - the reason it only gets triggered with the error_index_generator filter is because normally, an earlier Step will happen to call ensure(tool::Rustdoc), so the dynamic GHA check never gets run.

    this assertion is always guaranteed to show up at runtime pretty much constantly anytime someone accidentally adds additional nesting with a group

    correct. i was hoping that the check was eager enough they'd notice immediately and fix it in order to make CI pass, but this issue shows that it's non-trivial to do so because of the way it depends on the cache. this is how dry-run gets involved - when it's enabled, bypassing the cache is cheap and we can just do it unconditionally, which would have caught this bug before it got merged.

    I could see that the right fix is to track the group and end + restart it so that there's no nesting and instead a linear non-nested list of groups...

    hmm, that might handle our current cases ok, but it's not correct in general - consider something like

    let _guard = builder.msg("test rustdoc");
    run_cargo_test(compiler, "-p librustdoc"); // takes e.g. 60 seconds
    builder.ensure(RustdocUi); // builds rustdoc internally

    that will split up the single message before and after, which doesn't seem quite right, although i suppose it might be better than panicking?

    ::group::test rustdoc
    // 60 seconds
    ::endgroup
    ::group::build rustdoc
    ::endgroup
    ::group::test rustdoc
    // takes non-trivial time to run tests/rustdoc-ui suite
    ::endgroup
    
11. Mark-Simulacrum commented on Jul 18, 2023

    

    Mark-Simulacrum

    on Jul 18, 2023

    Member

    More actions

    Ah, I see. I'm not convinced that we'd not stop catching other problems and/or introduce them by changing dry run.

    The split example you suggest feels right to me - in some sense more right - it means we are putting things in finest granularity groups. It also matches what we do for metrics emission, I believe...

    I will say that I don't really understand the value in the grouping, so perhaps that would change my thinking there....

    At minimum, we might consider not panicking outside of GHA workloads, since the grouping doesn't matter then?
12. matthiaskrgr

    added 2 commits that reference this issue on Jul 20, 2023

    Rollup merge of rust-lang#113800 - oli-obk:gha_ci_cycle, r=jyn514

    ...

    4497c9b

    Rollup merge of rust-lang#113800 - oli-obk:gha_ci_cycle, r=jyn514

    ...

    6102785
13. bors

    closed this as completedin 6102785on Jul 20, 2023
14. apiraino

    removed

    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}Issue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}

    on Aug 1, 2023