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

fix bug of check-git-abc #4450

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix bug of check-git-abc #4450

wants to merge 1 commit into from

Conversation

nlwmode
Copy link

@nlwmode nlwmode commented Jun 13, 2024

This project distinguishes the yosyshq/abc and berkeley-abc by .gitcommit file that checked in check-git-abc function in Makefile.

However, the second and third "elif" branches' judgment conditions are reversed. And "grep -q '$$Format:%h$$' "$(YOSYS_SRC)/abc/.gitcommit" can check that there is a .gitcommit file in yosys/abc with the text "Format:%h".

This pull request aims to ensure that the second and third "elif" conditions produce the expected compilation results for using yosyshq/abc.

@nlwmode
Copy link
Author

nlwmode commented Jun 13, 2024

This PR is to solve the issue: #4449

@widlarizer
Copy link
Collaborator

As others have pointed out on the linked issue, I'm confirming that the problem can't be reproduced. Furthermore, the functionality you're changing the logic for in this PR actually works for me the way I'd expect at yosys main branch commit 1288166:

emil@fridge /t/a/yosys (main)> git submodule deinit abc
Cleared directory 'abc'
Submodule 'abc' (https://github.com/YosysHQ/abc) unregistered for path 'abc'
emil@fridge /t/a/yosys (main)> make
Initialize the submodule: Run 'git submodule update --init' to set up 'abc' as a submodule.
make: *** [Makefile:770: check-git-abc] Error 1
emil@fridge /t/a/yosys (main) [2]> rm -rf abc
emil@fridge /t/a/yosys (main)> cp -r ../abc/abc-yosys-0.42/ ./abc
emil@fridge /t/a/yosys (main)> cat abc/.gitcommit
237d81397f
emil@fridge /t/a/yosys (main)> make
'abc' comes from a tarball. Continuing.

Without more context we'll probably close the PR and issue fairly soon

@widlarizer widlarizer added the needs-info Issue needs more context/information in order to be resolved label Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-info Issue needs more context/information in order to be resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants