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.

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

Add option to control max build concurrency #577

Merged
merged 4 commits into from
Jan 24, 2025
Merged

Conversation

lynzrand
Copy link
Collaborator

@lynzrand lynzrand commented Jan 24, 2025

Related Issues

  • Related issues: #____

Type of Pull Request

  • Bug fix
  • New feature
    • I have already discussed this feature with the maintainers.
  • Refactor
  • Performance optimization
  • Documentation
  • Other (please describe):

Does this PR change existing behavior?

  • Yes (please describe the changes below)
    • Add -j<N> commandline arg to control parallelism
    • Add an environment variable MOON_MAX_PAR_TASKS to limit the maximum tasks building in parallel
  • No

Does this PR introduce new dependencies?

  • No
  • Yes (please check binary size changes)

Checklist:

  • Tests added/updated for bug fixes or new features
  • Compatible with Windows/Linux/macOS

Copy link

peter-jerry-ye-code-review bot commented Jan 24, 2025

‼️ This code review is generated by a bot. Please verify the content before trusting it.

⚠️ [Incorrect handling of maximum parallelism limit]
  • Category: Correctness
  • Code Snippet: crates/moonbuild/src/entry.rs in get_parallelism function
  • Recommendation: Ensure the environment variable MOON_MAX_PAR_TASKS acts as an upper bound. Modify the logic to:
    let mut parallelism = default_parallelism().unwrap_or(1);
    if let Ok(env_max) = std::env::var("MOON_MAX_PAR_TASKS") {
        let env_max = env_max.parse().context("...")?;
        parallelism = parallelism.min(env_max);
    }
    if let Some(cli_jobs) = opt.parallelism {
        parallelism = parallelism.min(cli_jobs);
    }
    Ok(parallelism)
  • Reasoning: The current implementation allows CLI-specified jobs to exceed the environment variable limit, which contradicts the PR description stating the environment variable should "limit the maximum tasks". This could lead to unintended over-parallelization.
⚠️ [Potential test data contamination]
  • Category: Correctness
  • Code Snippet: test_moon_parallelism() using TestDir::new("test_filter_pkg_with_test_imports.in")
  • Recommendation: Change test directory name to match test purpose:
    TestDir::new("test_parallelism.in")
  • Reasoning: The test appears to reuse another test's input directory (test_filter_pkg...), which may lead to incorrect test setup and unreliable results. Tests should have isolated, purpose-specific data.
💡 [Undocumented conflict between --no-parallelize and -j]
  • Category: Maintainability
  • Code Snippet: no_parallelize flag vs parallelism parameter
  • Recommendation: Either:
    1. Make --no-parallelize override -j by setting parallelism to 1
    2. Add validation to reject simultaneous use
    3. Update documentation to clarify behavior
  • Reasoning: The test uses both -j1 and --no-parallelize, suggesting possible confusion. The current codebase allows these flags to coexist without explicit handling, which could lead to unexpected behavior if values conflict.

@lynzrand lynzrand changed the title Add env var to get max concurrency for build Add option to control max build concurrency Jan 24, 2025
@lynzrand lynzrand requested review from Young-Flash and lijunchen and removed request for Young-Flash January 24, 2025 03:45
@lynzrand lynzrand requested a review from lijunchen January 24, 2025 06:08
@lynzrand lynzrand marked this pull request as ready for review January 24, 2025 06:38
@lijunchen lijunchen added this pull request to the merge queue Jan 24, 2025
@lynzrand lynzrand removed this pull request from the merge queue due to a manual request Jan 24, 2025
@lijunchen lijunchen added this pull request to the merge queue Jan 24, 2025
Merged via the queue into main with commit 258dd9e Jan 24, 2025
14 checks passed
@lijunchen lijunchen deleted the rynco/no-parallel branch January 24, 2025 11:13
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