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: gowin nextpnr build #10

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

Add: gowin nextpnr build #10

wants to merge 1 commit into from

Conversation

racerxdl
Copy link
Contributor

This adds support for gowin nextpnr. I tested successfully with https://github.com/racerxdl/tangnano-yosis-hello locally, just had to change the docker images for my local ones.

Smoke tests on hdl/smoke-tests#1, will update soon after merge and change this from draft to pull request

@racerxdl
Copy link
Contributor Author

racerxdl commented Jan 14, 2021

Omg, I forgot the +x on the test script on smoke-tests. Fixing it hdl/smoke-tests#2

Copy link
Member

@umarcor umarcor left a comment

Choose a reason for hiding this comment

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

First off, thanks for contributing! It looks good for a first approach, but we need to clarify a few things. Please see the comments below.

Do not worry about test 'impl' failing for now. If you check the graph (https://hdl.github.io/containers/#_graph) it depends on image nextpnr, so it will keep failing until this PR is done and merged. That's ok.

@@ -46,12 +46,14 @@ jobs:
- run: dockerBuild nextpnr:ice40 nextpnr ice40
- run: dockerBuild nextpnr:icestorm nextpnr icestorm
- run: dockerBuild nextpnr:ecp5 nextpnr ecp5
- run: dockerBuild nextpnr:gowin nextpnr gowin
Copy link
Member

Choose a reason for hiding this comment

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

Please, place gowin below prjtrellis. Those are couples:

  • ice40 and icestorm
  • ecp5 and prjtrellis
  • gowin and apicula

FROM build AS build-gowin
RUN mkdir -p /tmp/nextpnr/build \
&& apt-get update -qq \
&& DEBIAN_FRONTEND=noninteractive apt-get -y install --no-install-recommends python3-setuptools python3-pip \
Copy link
Member

Choose a reason for hiding this comment

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

I need some background about apicula/apycula. When building nextpnr-ice40 and nextpnr-ecp5, some assets are required, but not executables/scripts. If that is the case with nextpnr-gowin, then you should get the assets here instead of installing the Python package.

Apart from that, icestorm and prjtrellis are built in separated dockerfiles/workflows, from sources. Overall, each tool is built/installed once only. We should do the same for apicula/apycula. Then, this stage would COPY from hdlc/pkg:apicula.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually is a python library which also provides a link for running it as a executable. The nextpnr-gowin needs the library itself so it would just save some symbolic links.

I will make the adjusts to be at pkg and then just copy :D

Copy link
Member

Choose a reason for hiding this comment

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

Actually is a python library which also provides a link for running it as a executable. The nextpnr-gowin needs the library itself so it would just save some symbolic links.

Does nextpnr-gowin actually call apicula/apycula Python scripts at runtime? I would expect it to be required when building nextpnr-gowin, but then it should be independent (as nextpnr-ice40 or nextpnr-ecp5).

I will make the adjusts to be at pkg and then just copy :D

Thanks! You can take Symbiyosys' dockerfile as an example. However, that one has an explicit make install, which makes it easier to handle regardless of PIP. I had a quick look at apicula and I didn't find a similar feature.

Note that, when creating the package, the PREFIX of the tools needs to be /usr/local; and DESTDIR should be used for placing them in a temporary location (to be then copied to the package image). I don't know whether the PREFIX is relevant for apicula.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checked, after building it it doesnt need it. So I removed from the nextpnr container. Apicula is python so there is no make install, just pip and easy_install which doesnt support prefix, only target which does not generate the "binaries".


#---

FROM base AS gowin
Copy link
Member

Choose a reason for hiding this comment

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

There should be two stages here:

FROM base AS gowin

and

FROM gowin AS apicula
COPY --from=hdlc/pkg:apicula /apicula /

The first one would contain artifacts of nextpnr-gowin only; and the second one would include apicula/apycula too.

@@ -110,3 +138,6 @@ RUN cd /tmp/nextpnr/build \

FROM base AS all
Copy link
Member

Choose a reason for hiding this comment

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

This last "all" stage should include all nextpnr-* artifacts: nextpnr-ice40, nextpnr-ecp5, nextpnr-gowin and nextpnr-generic. Maybe it would be desirable to change this to build generic only, since others are built before:

#---

FROM build AS build-all

RUN cd /tmp/nextpnr/build \
 && cmake .. \
   -DARCH=generic \
   -DBUILD_GUI=OFF \
   -DBUILD_PYTHON=ON \
   -DUSE_OPENMP=ON \
 && make -j $(nproc) \
 && make DESTDIR=/opt/nextpnr install

#---

FROM base AS all
COPY --from=build-ice40 /opt/nextpnr /
COPY --from=build-ecp5 /opt/nextpnr /
COPY --from=build-gowin /opt/nextpnr /
COPY --from=build-generic /opt/nextpnr /

Note that this last image does not include icestorm, prjtrellis or apicula/apycula. We can later add another image for that, on top of this stage. But I think it's better to handle that after this PR is done.

Copy link
Member

Choose a reason for hiding this comment

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

Updating build-all to build-generic was contributed by @se-bi. Adding gowin after (#11) should be more straightforward now.

@racerxdl
Copy link
Contributor Author

hmm, maybe since it will need hdlc/apicula, I should first make a PR adding apicula as a base image?

@umarcor
Copy link
Member

umarcor commented Jan 15, 2021

hmm, maybe since it will need hdlc/apicula, I should first make a PR adding apicula as a base image?

That makes sense. hdlc/pkg:apicula and hdlc/apicula might be used regardless of nextpnr. Therefore, it will be cleaner to handle that first.

BTW, @se-bi is modifying impl images, which depend on nextpnr. You might want to coordinate. I would propose:

  1. Split git changes from Update impl-image, tests and enable tool version strings #4.
  2. hdlc/apicula and hdlc/pkg:apicula.
  3. Add nextpnr-gowin and rework nextpnr.dockerfile in this PR.
  4. In Update impl-image, tests and enable tool version strings #4, update impl images without need to touch nextpnr.

@umarcor umarcor added enhancement New feature or request $R/impl Related to `$R/impl` images and their dependencies $R/pkg Related to `$R/pkg` images and their dependencies labels Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request $R/impl Related to `$R/impl` images and their dependencies $R/pkg Related to `$R/pkg` images and their dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants