Skip to content

Conversation

shs96c
Copy link
Member

@shs96c shs96c commented Aug 22, 2024

User description

Rather than passing --stamp everywhere, use a proper config so that we can easily add additional flags.

I've broken out a remote_release config which builds things remotely and then downloads the artifacts given on the command line to the local machine. This only makes sense for targets which we do not run (as run happens locally, so we'd need to rebuild any artifacts anyway)


PR Type

enhancement


Description

  • Enhanced Bazel configurations by adding --compilation_mode=opt to the build:release and introducing a new build:remote_release configuration for remote builds.
  • Updated Rake tasks to replace --stamp with --config=release, ensuring consistent use of Bazel release configurations across different language tasks.
  • Modified tasks to use --config=remote_release for remote execution, improving build efficiency.

Changes walkthrough 📝

Relevant files
Enhancement
.bazelrc
Enhance Bazel configurations for release builds                   

.bazelrc

  • Added --compilation_mode=opt to build:release.
  • Introduced build:remote_release configuration for remote builds.
  • +7/-1     
    Rakefile
    Update Rake tasks to use Bazel release configs                     

    Rakefile

  • Replaced --stamp with --config=release in various tasks.
  • Updated tasks to use --config=remote_release for remote execution.
  • +12/-11 

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Rather than passing `--stamp` everywhere, use a proper config
    so that we can easily add additional flags.
    
    I've broken out a `remote_release` config which builds things
    remotely and then downloads the artifacts given on the command
    line to the local machine. This only makes sense for targets
    which we do _not_ `run` (as `run` happens locally, so we'd
    need to rebuild any artifacts anyway)
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Configuration Change
    The new remote_release configuration combines release and remote configs. Ensure this doesn't cause unexpected behavior in the build process.

    Build Configuration
    Verify that replacing --stamp with --config=release or --config=remote_release maintains all necessary build information and doesn't break any existing functionality.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Error prevention
    Add checks for required environment variables in the java:release task

    In the java:release task, consider adding a check to ensure that the required
    environment variables (MAVEN_REPO and GPG_SIGN) are set before proceeding with the
    release process. This can help prevent issues caused by missing configuration.

    Rakefile [871-876]

     ENV['MAVEN_REPO'] = "https://oss.sonatype.org/#{repo}"
     ENV['GPG_SIGN'] = (!nightly).to_s
    +
    +raise "MAVEN_REPO is not set" if ENV['MAVEN_REPO'].nil? || ENV['MAVEN_REPO'].empty?
    +raise "GPG_SIGN is not set" if ENV['GPG_SIGN'].nil? || ENV['GPG_SIGN'].empty?
     
     Rake::Task['java:version'].invoke if nightly
     Rake::Task['java:package'].invoke('--config=release')
     Rake::Task['java:build'].invoke('--config=release')
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Ensuring that required environment variables are set before proceeding with the release process is crucial for preventing issues caused by missing configuration, making this a high-priority improvement.

    9
    Add a check for the NUGET_API_KEY environment variable in the dotnet:release task

    In the dotnet:release task, consider adding a check to ensure that the NUGET_API_KEY
    environment variable is set before attempting to use it. This can help prevent
    errors and provide a more informative message if the key is missing.

    Rakefile [758-760]

     api_key = ENV.fetch('NUGET_API_KEY', nil)
    +raise "NUGET_API_KEY environment variable is not set" if api_key.nil?
     push_destination = 'https://api.nuget.org/v3/index.json'
     if nightly
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Checking for the NUGET_API_KEY environment variable before use is essential for preventing errors and providing informative messages if the key is missing, significantly improving error prevention.

    9
    Error handling
    Add error handling to the java-release-zip task

    Consider adding error handling for the Bazel.execute call in the java-release-zip
    task. This will help catch and report any issues that may occur during the build
    process.

    Rakefile [333-335]

     task :'java-release-zip' do
    -  Rake::Task['java:package'].invoke('--config=remote_release')
    +  begin
    +    Rake::Task['java:package'].invoke('--config=remote_release')
    +  rescue StandardError => e
    +    puts "Error during java-release-zip task: #{e.message}"
    +    raise
    +  end
     end
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling for the Bazel.execute call in the java-release-zip task is important for catching and reporting issues during the build process, enhancing robustness and reliability.

    8
    Maintainability
    Add a comment to explain the purpose of the release configuration

    Consider adding a comment explaining the purpose of the release configuration,
    especially the --compilation_mode=opt flag. This will help other developers
    understand the optimization level used for release builds.

    .bazelrc [106-107]

    +# Release configuration: Enables build stamping and optimized compilation
     build:release --stamp
     build:release --compilation_mode=opt
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Adding a comment to explain the purpose of the release configuration improves code maintainability and helps other developers understand the optimization level used for release builds, but it is not crucial for functionality.

    5

    @nvborisenko
    Copy link
    Member

    Verified locally, it fixes #14405

    @shs96c shs96c merged commit e2f6e14 into SeleniumHQ:trunk Aug 26, 2024
    @shs96c shs96c deleted the release-config branch August 26, 2024 09:04
    M1troll pushed a commit to M1troll/selenium that referenced this pull request May 14, 2025
    …4423)
    
    Rather than passing `--stamp` everywhere, use a proper config
    so that we can easily add additional flags.
    
    I've broken out a `remote_release` config which builds things
    remotely and then downloads the artifacts given on the command
    line to the local machine. This only makes sense for targets
    which we do _not_ `run` (as `run` happens locally, so we'd
    need to rebuild any artifacts anyway)
    
    Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants