Skip to content

etc: Use _execute for bison download to get log only on failure as for others.#10777

Open
titan73 wants to merge 2 commits into
The-OpenROAD-Project:masterfrom
titan73:master
Open

etc: Use _execute for bison download to get log only on failure as for others.#10777
titan73 wants to merge 2 commits into
The-OpenROAD-Project:masterfrom
titan73:master

Conversation

@titan73

@titan73 titan73 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

On success:
[INFO] Downloading Bison ✔

On failure:

INFO] Downloading Bison ✖
[INFO] Trying to download bison from: https://ftp.gnu.org/gnu/bison/bison-3.8.2.tar.gz
--2026-06-29 15:53:34--  https://ftp.gnu.org/gnu/bison/bison-3.8.2.tar.gz
Resolving ftp.gnu.org (ftp.gnu.org)... failed: Name or service not known.
wget: unable to resolve host address ‘ftp.gnu.org’
[WARNING] Failed to download from https://ftp.gnu.org/gnu/bison
[INFO] Trying to download bison from: https://ftpmirror.gnu.org/bison/bison-3.8.2.tar.gz
--2026-06-29 15:53:34--  https://ftpmirror.gnu.org/bison/bison-3.8.2.tar.gz
Resolving ftpmirror.gnu.org (ftpmirror.gnu.org)... failed: Name or service not known.
wget: unable to resolve host address ‘ftpmirror.gnu.org’
[WARNING] Failed to download from https://ftpmirror.gnu.org/bison
[INFO] Trying to download bison from: https://mirrors.kernel.org/gnu/bison/bison-3.8.2.tar.gz
--2026-06-29 15:53:34--  https://mirrors.kernel.org/gnu/bison/bison-3.8.2.tar.gz
Resolving mirrors.kernel.org (mirrors.kernel.org)... failed: Name or service not known.
wget: unable to resolve host address ‘mirrors.kernel.org’
[WARNING] Failed to download from https://mirrors.kernel.org/gnu/bison
[INFO] Trying to download bison from: https://mirrors.dotsrc.org/gnu/bison/bison-3.8.2.tar.gz
--2026-06-29 15:53:34--  https://mirrors.dotsrc.org/gnu/bison/bison-3.8.2.tar.gz
Resolving mirrors.dotsrc.org (mirrors.dotsrc.org)... failed: Name or service not known.
wget: unable to resolve host address ‘mirrors.dotsrc.org’
[WARNING] Failed to download from https://mirrors.dotsrc.org/gnu/bison
[ERROR] Could not download bison-3.8.2.tar.gz from any mirror.
[ERROR] Failed to execute: _download_bison

@titan73 titan73 requested a review from a team as a code owner June 29, 2026 14:05
@titan73 titan73 requested a review from precisionmoon June 29, 2026 14:05
…r others.

Signed-off-by: Christian Costa <titan.costa@gmail.com>

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request refactors the Bison download logic in etc/DependencyInstaller.sh by wrapping it in a local function _download_bison and running it via _execute. The review feedback points out that using a subshell wrapper (error ...) to prevent immediate exit is an anti-pattern, suggesting instead to print the error to stderr and return a non-zero exit status to signal failure.

Comment on lines +359 to 361
if [[ ${success} -ne 1 ]]; then
(error "Could not download bison-${BISON_VERSION}.tar.gz from any mirror.")
fi

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.

medium

Using a subshell wrapper (error ...) to prevent the script from exiting immediately is an anti-pattern and can be confusing. Since _download_bison is executed via _execute (which handles logging and final termination on failure), the function should simply print the error message to stderr and return a non-zero exit status (e.g., return 1) to signal failure to the caller.

Suggested change
if [[ ${success} -ne 1 ]]; then
(error "Could not download bison-${BISON_VERSION}.tar.gz from any mirror.")
fi
if [[ ${success} -ne 1 ]]; then
echo "Could not download bison-${BISON_VERSION}.tar.gz from any mirror." >&2
return 1
fi

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. I used "warn ..." instead of an echo though.

As suggested by Gemini.
²
Signed-off-by: Christian Costa <titan.costa@gmail.com>
@titan73

titan73 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@maliberty

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

Reviewed commit: 5fa351d005

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@titan73

titan73 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@precisionmoon Any feedback on this PR?

@maliberty maliberty requested review from maliberty and removed request for precisionmoon July 2, 2026 16:39
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.

2 participants