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

CI: Windows/MSYS2 packaging #65

Merged
merged 10 commits into from
Jul 17, 2021
Merged

CI: Windows/MSYS2 packaging #65

merged 10 commits into from
Jul 17, 2021

Conversation

umarcor
Copy link
Contributor

@umarcor umarcor commented Nov 29, 2020

This PR is not to be merged per se, but for showcasing a problem.

In the context of hdl/MINGW-packages, I'm writing a PKGBUILD file for having openFPGALoader upstreamed as an MSYS2 package (packages.msys2.org). See hdl/MINGW-packages@openFPGALoader: mingw-w64-openFPGALoader.

This PR contains a PKGBUILD similar to the one in hdl/MINGW-packages, but here sources are not retrieved, since the build is executed in subdir msys2 (new, where PKGBUILD is located). Furthermore, a GitHub Actions workflow is added. It uses Action setup-msys2, mimicking the workflow in the upstream msys2/MINGW-packages.

So far, building is successful. However, make install fails. Locally, it will open an interactive prompt. Upon exit, the build will fail. On the other hand, in CI it cannot open the interactive prompt, so it is skipped and nothing is installed (packaged). In this PR, I worked around it by manually copying the files in https://github.com/trabucayre/openFPGALoader/blob/master/CMakeLists.txt#L174-L182. Still, I'd like to fix it.

I checked open-tool-forge/fpga-toolchain (/cc @edbordin), but it seems that make install is not used there: https://github.com/open-tool-forge/fpga-toolchain/blob/main/scripts/compile_openfpgaloader.sh#L34.

@trabucayre, is the location of those bitstreams and test_sfl.svf correct?

See CI run: https://github.com/umarcor/openFPGALoader/actions/runs/389784436

@umarcor umarcor force-pushed the ci/msys2 branch 2 times, most recently from b4ab050 to 2439021 Compare November 29, 2020 08:55
@trabucayre
Copy link
Owner

Amazing!
Great work!
Yes. I'm not happy if a manual installation must be done for (at least) msys2.
My skill with msys2 is near to 0, so I'm quite unable to provides an answer but yes this issue must be resolved
Maybe something like:

-install(TARGETS openFPGALoader DESTINATION bin)
+install(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/openFPGALoader DESTINATION bin)

may resolve this issue (seems works for libftdi)

bitstreams (including test_sfl.svf) must be installed in
_data="${pkgdir}${MINGW_PREFIX}/share/${_realname}"(without data subdirectory).

Thanks

@umarcor
Copy link
Contributor Author

umarcor commented Nov 29, 2020

So, it was a silly mistake on my side... I was using make (for MSYS) instead of mingw32-make (for MINGW32/MINGW64). Changing that fixed the problem with make install. As a result, all the assets are now properly located.

I updated this PR. If you want, you can merge it and keep it for CI purposes. It will build one MINGW32 and one MINGW64 package after each push or pull request.

BTW, would you mind releasing a new version (git tag)? That would help for getting an archive when upstreaming the package (instead of requiring git).

@trabucayre
Copy link
Owner

Releasing a new version is in my todolist. Current release v0.1 is outdated. I need to check if some issue or idea needs to be fixed / applied before or after new release. But a new release must be done ASAP.
For this PR I hesitate. It's a good thing to have a CI to check failure but it's requires to have this for all systems. PKGBUILD is required? It's usually more in the external build system no?

@umarcor
Copy link
Contributor Author

umarcor commented Dec 1, 2020

Releasing a new version is in my todolist. Current release v0.1 is outdated. I need to check if some issue or idea needs to be fixed / applied before or after new release. But a new release must be done ASAP.

No rush, since we can use git meanwhile (msys2/MINGW-packages#7351), but it'd be nice to retrieve a tarball instead.

For this PR I hesitate. It's a good thing to have a CI to check failure but it's requires to have this for all systems.

I agree. hdl/MINGW-packages is for MSYS2 packages. There is a sibling project, hdl/containers, for OCI containers (docker, podman, etc.). That's a container ecosystem of EDA tooling, based on Debian Buster. Hence, it would be possible to use the same plumbing for CI here. In fact, hdl/containers is an evolution of the CI infrastructure in GHDL. See https://hdl.github.io/containers/#_context.

Furthermore, it is possible to use docker containers for foreign architectures (say ARM). That allows generating multiarch images. See dbhi/qus and dbhi/docker. Actually, dbhi/qus is used in open-tool-forge/fpga-toolchain for building static binaries of this project (see https://github.com/open-tool-forge/fpga-toolchain/blob/feature/46-arm-builds/scripts/test/test_entrypoint.sh#L24).

PKGBUILD is required? It's usually more in the external build system no?

See a discussion about different packaging approaches in https://github.com/hdl/smoke-tests/blob/main/CONTEXT.md (refed from https://github.com/hdl/MINGW-packages/blob/main/CONTEXT.md).

When packaging a tool for typical GNU/Linux distributions (apt, dnf/yum, pacman...) the complexity relies on the number of target architectures that need to be supported. Each tool needs to be cross-compiled for 6-12 different platforms, which increases the maintenance burden significantly. Conversely, when packaging for Windows, there are de facto two options only (MINGW32 or MINGW64) and a single package manager exists. Therefore, the variability and effort are much less.

Moreover, the point about PKGBUILD files is that those are probably the most simple building/packaging recipes I am aware of. pacman is not an MSYS2 tool, but the default package manager in Arch Linux. MSYS2 borrows the same straightforward approach. PKGBUILD files are plain bash scripts. ~30/40 lines in the proposed PKGBUILD file would remain unchanged if you wrote the process manually, instead of using pacman for building:

# set pkgdir
# set MINGW_PACKAGE_PREFIX

mkdir "${pkgdir}"

_realname=openFPGALoader

pacman -S --noconfirm \
  "${MINGW_PACKAGE_PREFIX}-libftdi" \
  "${MINGW_PACKAGE_PREFIX}-gcc" \
  "${MINGW_PACKAGE_PREFIX}-make" \
  "${MINGW_PACKAGE_PREFIX}-cmake")

mkdir build
cd build
MSYS2_ARG_CONV_EXCL=- cmake \
  -G "MinGW Makefiles" \
  -DCMAKE_INSTALL_PREFIX="${MINGW_PREFIX}" \
  ../
cmake --build .

openFPGALoader.exe --help

mingw32-make DESTDIR="${pkgdir}" install

_licenses="${pkgdir}${MINGW_PREFIX}/share/licenses/${_realname}"
mkdir -p "${_licenses}"
install -m 644 ../LICENSE "${_licenses}"

The remaining 10 lines are the wrapping functions (build, check, package) and metadata (name, version, description, url, license). Yet, manually you would need to handle:

  • Different variables for 32 and 64 bits.
  • Packaging (generation of tarballs).
  • Communicating dependencies to users.

By using makepkg instead (well, precisely makepkg-mingw), all that is automatically handled. The resulting *.zst tarballs can then be either extracted with tar, or installed through pacman. The latter will install dependencies automatically.

Last, but not least, having a PKGBUILD file is useful for:

  • Letting any user build master locally with a single command: MINGW_INSTALLS=mingw64 makepkg-mingw.
  • Synchronising with upstream. Preferredly, dealing with patches in the codebase (if required), instead of waiting until the package is bumped for watching problems arise.

For reference, in GHDL, PKGBUILD files are used (https://github.com/ghdl/ghdl/tree/master/dist/msys2-mingw) for building nightly artifacts (https://github.com/ghdl/ghdl/runs/1472008237?check_suite_focus=true and https://github.com/ghdl/ghdl/actions/runs/390849319), which are then available in a pre-release (https://github.com/ghdl/ghdl/releases/tag/nightly) and through a GitHub Action (https://github.com/ghdl/setup-ghdl-ci). The PKGBUILD files in GHDL's repo are similar but not the same as the upstream (https://github.com/msys2/MINGW-packages/tree/master/mingw-w64-ghdl).

Overall, I believe that using PKGBUILD files for MSYS2 is easier to understand and maintain than any manual procedure. Yet, please, feel free to argue otherwise.

@umarcor
Copy link
Contributor Author

umarcor commented Dec 9, 2020

openFPGALoader was upstreamed to MSYS2 (https://github.com/msys2/MINGW-packages/tree/master/mingw-w64-openFPGALoader) and it's in the update queue: https://packages.msys2.org/new. When released (removed from the update queue), it will be available through pacman -S mingw-w64-x86_64-openFPGALoader or pacman -S mingw-w64-i686-openFPGALoader. See hdl/MINGW-packages.

@umarcor
Copy link
Contributor Author

umarcor commented Mar 28, 2021

@trabucayre I rebased on top of master and all looks good. Do you mind tagging v0.2.6 for updating the MSYS2 packages? https://packages.msys2.org/package/mingw-w64-x86_64-openFPGALoader

@trabucayre
Copy link
Owner

Sorry for delay.
It's done now
Thanks

@umarcor
Copy link
Contributor Author

umarcor commented Apr 7, 2021

No worries and thanks!
The package is updated (msys2/MINGW-packages#8300) and ready to be uploaded to official repos: https://packages.msys2.org/queue

@trabucayre
Copy link
Owner

Great!
Thanks!

@trabucayre
Copy link
Owner

hi,
msys2 directory must be at the top level or it's possible to move this into .github ?
I'm not aware with action and autotest but it's possible with starting with your workflows to add more target (debian, ubuntu, macos, ...)?

I think I must apply this PR to be sure to have a buildable system everytime for all possible target.
Thanks

@umarcor
Copy link
Contributor Author

umarcor commented Jul 16, 2021

msys2 directory must be at the top level or it's possible to move this into .github ?

Not at all. The PKGBUILD recipe can be anywhere, and the directory does not need to be named msys2. For instance:

  • In GHDL: scripts/msys2-llvm/PKGBUILD and scripts/msys2-mcode/PKGBUILD.
  • In GTKWave: MSYS2/gtk2/PKGBUILD and MSYS2/gtk3/PKGBUILD.
  • In iverilog: msys2/PKGBUILD.
  • In MINGW-packages: mingw-w64-*/PKGBUILD.

The only point is that you need to mv to the location of the PKGBUILD recipe and then execute makepkg-mingw there.

On the other hand, I don't think that build recipes belong in .github. Those can and should also be used locally by contributors. That is how I build openFPGALoader locally for testing. So, it's ok to move them into the subdir you want, but I would advice to have it outside of .github.

I'm not aware with action and autotest but it's possible with starting with your workflows to add more target (debian, ubuntu, macos, ...)?

Yes, it is possible. Not exactly the same job, but we can add other jobs or additional workflows.
See, for instance, the workflow in GHDL:

Here, it will be difficult to actually test the tool in CI. But we can build it on multiple platforms and ensure that it does not crash, at least (https://github.com/hdl/smoke-tests/blob/main/openFPGALoader.sh).

Overall, I don't like manual build procedures. That's why I use PKGBUILD recipes on MSYS2. Hence, I am not comfortable with using manual procedures on Linux jobs. I'd prefer to use DEB or RPM recipes. Yet, I am lacking knowledge about those specific recipe formats. Hence, unfortunately, I cannot help in that regard. I can help with the CI/workflow/artifact management, though (as I do in GHDL, GTKWave, etc.).

BTW, as soon as I get familiar with building openFPGALoader on Debian Buster, I want to add it to the prog container images from hdl/containers (see https://hdl.github.io/containers/#_to_do).

I think I must apply this PR to be sure to have a buildable system everytime for all possible target.

I agree. CI is a must nowadays. Software development is very organic, and small projects need all the help from automation they can get. That is one of the roles I play in the open source EDA tooling community. Hence, I'm willing to walk this path with you. However, it needs to be a tight collaboration because I cannot afford the cognitive load of learning the specific details of this project.

So, if you are willing to merge this PR, let me know, and I will review it before doing so. Do you like emojis? We use those a lot in GHDL and in other repos, because it helps understand the big picture:

image

However, those are unicode characters, so I understand if some projects don't want to use them.

@trabucayre
Copy link
Owner

The only point is that you need to mv to the location of the PKGBUILD recipe and then execute makepkg-mingw there.

Ok

On the other hand, I don't think that build recipes belong in .github. Those can and should also be used locally by contributors. That is how I build openFPGALoader locally for testing. So, it's ok to move them into the subdir you want, but I would advice to have it outside of .github.

It make sense. scripts directory seems a good location.

Here, it will be difficult to actually test the tool in CI. But we can build it on multiple platforms and ensure that it does not crash, at least (https://github.com/hdl/smoke-tests/blob/main/openFPGALoader.sh).

I needs to learn more about this task, it's really interesting and important to be sure to haven't regressions.

Overall, I don't like manual build procedures. That's why I use PKGBUILD recipes on MSYS2. Hence, I am not comfortable with using manual procedures on Linux jobs. I'd prefer to use DEB or RPM recipes. Yet, I am lacking knowledge about those specific recipe formats. Hence, unfortunately, I cannot help in that regard. I can help with the CI/workflow/artifact management, though (as I do in GHDL, GTKWave, etc.).

It's true. It's a more clean way to install package.

BTW, as soon as I get familiar with building openFPGALoader on Debian Buster, I want to add it to the prog container images from hdl/containers (see https://hdl.github.io/containers/#_to_do).

Building manually openFPGALoader in debian is really straightforward. Will see how to create files to generate a deb. For RPM I think @jeanthom knows how to do this

I think I must apply this PR to be sure to have a buildable system everytime for all possible target.

I agree. CI is a must nowadays. Software development is very organic, and small projects need all the help from automation they can get. That is one of the roles I play in the open source EDA tooling community. Hence, I'm willing to walk this path with you. However, it needs to be a tight collaboration because I cannot afford the cognitive load of learning the specific details of this project.

Yes of course !

So, if you are willing to merge this PR, let me know, and I will review it before doing so. Do you like emojis? We use those a lot in GHDL and in other repos, because it helps understand the big picture:

It's my goal yes. it's true this display is more easy to understood as textual version

Thanks.

@umarcor umarcor force-pushed the ci/msys2 branch 2 times, most recently from baf4eea to bfb7a48 Compare July 17, 2021 10:03
* Split build and test into two jobs.
* Use emojis/icons.
* Build/test clang64 and ucrt64 too.
* Add workflow_dispatch.
@umarcor
Copy link
Contributor Author

umarcor commented Jul 17, 2021

@trabucayre I think this is ready to merge now.

  • Moved msys2 subdir into scripts/msys2.
  • Added emojis/icons to the workflow.
  • Split build and test jobs, to better reproduce what users will get. That is, do not pollute the test job with build dependencies/artifacts.
  • Build clang64 and ucrt64 variants too.
  • Add workflow_dispatch for manually triggering the workflow, should you want/need to do it.
  • Added a shield/badge to the README for showing the status of CI.

I suggest we merge this, so that GitHub Actions is enabled in this repo. From there on, PRs will trigger CI runs, which will make it easier to iterate. Meanwhile, see an execution: https://github.com/umarcor/openFPGALoader/actions/runs/1040103477

@trabucayre
Copy link
Owner

LGTM just two things:

  • why cd "${srcdir}"/../../.. it's possible to do cmake "${srcdir}"/../../.. ? Or is it not a good idea ?
  • you have added and SoCs in README.md. it's true there is a misc_dev_list containing a ref to a cortex A9 but it's only required to know irlength in multiple device context (zynq and SoCFPGA). openFPGALoader doesn't know how to program/deal with a CPU :)

Thanks

@umarcor umarcor marked this pull request as draft July 17, 2021 14:08
@umarcor umarcor marked this pull request as ready for review July 17, 2021 14:32
@umarcor
Copy link
Contributor Author

umarcor commented Jul 17, 2021

  • why cd "${srcdir}"/../../.. it's possible to do cmake "${srcdir}"/../../.. ? Or is it not a good idea ?

It's actually a good idea. I changed it.

  • you have added and SoCs in README.md. it's true there is a misc_dev_list containing a ref to a cortex A9 but it's only required to know irlength in multiple device context (zynq and SoCFPGA). openFPGALoader doesn't know how to program/deal with a CPU :)

My idea was to just finish the sentence with a dot. Then I thought about adding the and SoCs. Anyway, I removed it now.

@trabucayre
Copy link
Owner

Thanks.
Sorry I've seen a small detail: I've changed license now it's Apache-2.0 License
It's less important but @jeanthom in this PR use --detect as smoke test. This prove openFPGALoader communicate correctly with the libftdi (and consequently libusb). Is it make sense to do the same thing here?

@umarcor
Copy link
Contributor Author

umarcor commented Jul 17, 2021

I've changed license now it's Apache-2.0 License

Fixed now!

It's less important but @jeanthom in this PR use --detect as smoke test. This prove openFPGALoader communicate correctly with the libftdi (and consequently libusb). Is it make sense to do the same thing here?

I added it in the "Test package" step of the workflow, instead of the "check()" function of the PKGBUILD recipe. Is that ok? Or do you prefer to have it in the recipe?

@trabucayre trabucayre merged commit ef52dc3 into trabucayre:master Jul 17, 2021
@trabucayre
Copy link
Owner

Thanks!
Applied

@umarcor
Copy link
Contributor Author

umarcor commented Nov 28, 2021

@trabucayre, maybe you can tag 0.6.0 to update the MSYS2 packages?

@trabucayre
Copy link
Owner

I was thinking to finish spiflash's rework, and merge @UweBonnes bmp PR before next release, but it's make more sense to create a new release and to postpone these evolutions to the next one.

@trabucayre
Copy link
Owner

done!

@umarcor
Copy link
Contributor Author

umarcor commented Dec 1, 2021

Thanks! msys2/MINGW-packages#10224

@trabucayre
Copy link
Owner

Just seen this PR!
Unfortunately a bit too late: zlib is now required to have access to all spiOverJtag bitstreams for altera.
Thanks!

@umarcor
Copy link
Contributor Author

umarcor commented Dec 1, 2021

Good catch! That's actually not critical, because zlib is a dependency of gcc, cmake, etc. That's why CI did not fail.

Anyway, I will keep that fix in a branch, to be pushed the next time I bump other packages.

@trabucayre
Copy link
Owner

In fact zlib is not marked as required (in a first time) but I must change this.

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.

2 participants