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 exception handling for the synthesis CLI commands #127

Merged

Conversation

IAmMarcelJung
Copy link
Collaborator

The CLI always crashed when there was an error during synthesis (e.g. file not found or error in files). The traceback also "hid" the error with its output a few lines above. Now the user just gets notified that the synthesis failed, the error is just above the short logging message and the CLI does not crash so the user does not have to restart FABulous and reload the fabric after fixing the error. If it was just a typo in the path or file, the mistake can just be corrected from the history without completely retyping the command which is necessary after restarting FABulous.

Copy link
Contributor

@TaoBi22 TaoBi22 left a comment

Choose a reason for hiding this comment

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

Looks good to me! I also had issues with this testing #128 - I would guess it's probably worth making similar changes to the nextpnr call as well but I'm happy to put that in another PR. Requested review from Kelvin too since he wrote the CLI.

Copy link
Collaborator

@KelvinChung2000 KelvinChung2000 left a comment

Choose a reason for hiding this comment

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

This is looking good to me.

@KelvinChung2000 KelvinChung2000 merged commit e5d11bd into FPGA-Research:master Nov 10, 2023
2 checks passed
@IAmMarcelJung IAmMarcelJung deleted the feat_error_handling branch November 10, 2023 12:54
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.

3 participants