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 YosysHQ/abc as a submodule located in abc. #4243

Merged
merged 4 commits into from
May 8, 2024

Conversation

RCoeurjoly
Copy link
Contributor

@RCoeurjoly RCoeurjoly commented Feb 27, 2024

Add YosysHQ/abc as a submodule located in abc. Apart from cloning with submodules, no other modifications are made to github actions.

This would enable building yosys with abc fetched as tar.gz, as pointed out by @mmicko in #4238 (comment)

Before starting the compilation of abc, we check four cases:

  1. abc directory does not exist. The compilation is stopped with the following error.

Error: The 'abc' directory does not exist.
Initialize the submodule: Run 'git submodule update --init' to set up 'abc' as a submodule.

  1. abc is a clone of abc (abc/.gitcommit content is Format). The compilation is stopped with the following error.

Error: 'abc' is not configured as a git submodule.
To resolve this:

  1. Back up your changes: Save any modifications from the 'abc' directory to another location.
  2. Remove the existing 'abc' directory: Delete the 'abc' directory and all its contents.
  3. Initialize the submodule: Run 'git submodule update --init' to set up 'abc' as a submodule.
  4. Reapply your changes: Move your saved changes back to the 'abc' directory, if necessary.
  1. abc comes from a tarball (abc/.gitcommit content is different than $Format:%h$). The compilation can continue.

'abc' comes from a tarball. Continuing.

  1. abc is a git submodule (checked with git submodule status). The compilation can continue.

'abc' is a git submodule. Continuing.

@RCoeurjoly RCoeurjoly changed the title Add YosysHQ/abc as a submodule located in abc_submodule. Add symbolic… Add YosysHQ/abc as a submodule located in abc_submodule. Feb 27, 2024
@mmicko
Copy link
Member

mmicko commented Feb 27, 2024

@RCoeurjoly please note that WASI is intentionally built in build directory since that exercise out of tree build that is used in YoWASP distribution. That is then very useful since it points out if changes done in Makefiles affect out of tree builds.

@RCoeurjoly
Copy link
Contributor Author

@RCoeurjoly please note that WASI is intentionally built in build directory since that exercise out of tree build that is used in YoWASP distribution. That is then very useful since it points out if changes done in Makefiles affect out of tree builds.

Oh I see.

@RCoeurjoly RCoeurjoly marked this pull request as draft February 27, 2024 17:28
@mmicko
Copy link
Member

mmicko commented Feb 27, 2024

Also abc.tar.gz that is used is one from https://github.com/YosysHQ/yosys/releases This is more for situations where people are downloading specific Yosys version and corresponding abc.tar.gz file and building them on isolated environments.

.gitmodules Outdated Show resolved Hide resolved
Makefile Outdated
.PHONY: abc/abc-$(ABCREV)$(EXE)
.PHONY: abc/libabc-$(ABCREV).a
.PHONY: abc/abc$(EXE)
.PHONY: abc/libabc.a
Copy link
Member

Choose a reason for hiding this comment

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

$(ABCREV) was used to make sure abc actually gets rebuilt when you change the ABC checkout. Is this still the case? If not you'll need some other way to rebuild ABC when that happens.

@RCoeurjoly RCoeurjoly force-pushed the submodule_abc branch 2 times, most recently from fa2797a to 4d3250e Compare February 28, 2024 18:46
Makefile Outdated
@@ -776,15 +776,15 @@ $(PROGRAM_PREFIX)yosys-config: misc/yosys-config.in

ABC_SOURCES := $(wildcard $(YOSYS_SRC)/abc/*)

abc/abc$(EXE) abc/libabc.a: $(ABC_SOURCES)
$(YOSYS_SRC)/abc/abc$(EXE) $(YOSYS_SRC)/abc/libabc.a: $(ABC_SOURCES)
Copy link
Member

Choose a reason for hiding this comment

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

An out-of-tree build of Yosys should also do an out-of-tree build of abc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the abc executable be located in build in that case?

Copy link
Member

@whitequark whitequark Feb 28, 2024

Choose a reason for hiding this comment

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

Yep, along with all other files generated during the build. Running git clean -dxf in the Yosys or abc checkout should do nothing. (Modulo the odd case of abc being not actually a checked-out submodule, where generated files should live in the same place but my illustration does not work.)

Makefile Outdated Show resolved Hide resolved
@RCoeurjoly RCoeurjoly changed the title Add YosysHQ/abc as a submodule located in abc_submodule. Add YosysHQ/abc as a submodule located in abc. Feb 29, 2024
@RCoeurjoly RCoeurjoly marked this pull request as ready for review February 29, 2024 13:00
Makefile Outdated Show resolved Hide resolved
@RCoeurjoly RCoeurjoly force-pushed the submodule_abc branch 2 times, most recently from 07d478c to 4368804 Compare April 4, 2024 11:27
@RCoeurjoly RCoeurjoly requested a review from mmicko as a code owner April 15, 2024 13:23
@RCoeurjoly RCoeurjoly force-pushed the submodule_abc branch 2 times, most recently from 4920c5c to 93a5ec9 Compare April 16, 2024 14:50
@RCoeurjoly RCoeurjoly marked this pull request as draft April 17, 2024 14:42
.gitignore Outdated
@@ -46,3 +45,4 @@ __pycache__
/tests/unit/bintest/
/tests/unit/objtest/
/tests/ystests
build
Copy link
Member

Choose a reason for hiding this comment

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

Should this not be /build? This would ignore every file or directory called build in the entire tree, which might unpleasantly surprise someone.

Makefile Outdated
ABC_SOURCES := $(wildcard $(YOSYS_SRC)/abc/*)


# This is a relative path, that is, we would like to build abc in $ROOT/abc/abc if compiling from root or $ROOT/build/abc/abc if compiling from $ROOT/build
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit of a confusing comment, as it appears to imply there's no support for out-of-tree builds.

@RCoeurjoly RCoeurjoly marked this pull request as ready for review April 18, 2024 07:34
Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

Finally!!! Great job!

@RCoeurjoly
Copy link
Contributor Author

Finally!!! Great job!

Thanks! It was a team effort.
Also note that since you approved of the changes, I made a change to address @povik proposal here #4243 (comment).

You may want to review that also.

@RCoeurjoly RCoeurjoly force-pushed the submodule_abc branch 3 times, most recently from a6a6fda to 98d7475 Compare April 23, 2024 21:52
@RCoeurjoly RCoeurjoly requested a review from povik April 24, 2024 08:56
@RCoeurjoly RCoeurjoly force-pushed the submodule_abc branch 2 times, most recently from d548186 to 6d181c2 Compare May 8, 2024 12:29
@mmicko mmicko merged commit b37e8d5 into YosysHQ:main May 8, 2024
18 checks passed
@whitequark
Copy link
Member

This broke the YoWASP build after all: https://github.com/YoWASP/yosys/actions/runs/9010585766/job/24756882140#step:7:657

'abc' comes from a tarball. Continuing.
mkdir -p abc && make -C abc -f "" ABCSRC="/home/runner/work/yosys/yosys/yosys-src/abc"  CC="ccache clang" CXX="ccache clang" ABC_USE_LIBSTDCXX=1 ABC_USE_NAMESPACE=abc VERBOSE= AR="llvm-ar" RANLIB="llvm-ranlib" ARCHFLAGS="-target wasm32-wasi --sysroot /home/runner/work/yosys/yosys/wasi-sdk-19.0/share/wasi-sysroot  -D_WASI_EMULATED_PROCESS_CLOCKS -DABC_USE_STDINT_H -DABC_NO_DYNAMIC_LINKING -DABC_NO_RLIMIT" OPTFLAGS="-Os" "ABC_USE_NO_READLINE=1" "ABC_USE_NO_PTHREADS=1" PROG="abc" MSG_PREFIX="-> ABC: " libabc.a
make: the '-f' option requires a non-empty string argument
make: Entering an unknown directory
Usage: make [options] [target] ...

@whitequark
Copy link
Member

So it turns out the YoWASP build was broken because I didn't check out submodules recursively, and the really confusing error message comes from the combination of an improper check for the presence of the submodule combined with the use of realpath.

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.

4 participants