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

Automatic creation of .deb packages #72

Merged
merged 34 commits into from
Dec 13, 2021

Conversation

boris-martin
Copy link
Collaborator

@boris-martin boris-martin commented Oct 21, 2021

This adds a Github Actions workflow that download dependencies and Calculix source code, builds the adapter (the "regular" version, without PaStiX), creates a .deb package and uploads it as artifact. (It can then be downloaded from Github on the workflow run)
Currently runs on Ubuntu 18.04 and 20.04 (The 2 latest LTS versions). It should work on other versions but I couldn't get Github to run on 21.04, and it stalled the entire workflow forever so I removed it.

Usage : download the corresponding artifact and sudo apt install [packagefile]. This requires an installation of preCICE through the provided packages (i.e. this package won't work with a custom build of preCICE, as it explicitly requires the libprecice package)

The makefile has been updated to be similar (just replaced 2.16 by 2.17 everywhere) to the one of the master branch instead of the current v2.17, which is outdated and painful to use, especially within scripts.

Things yet to do :

  • Write some stuff in the man page. There is a script that converts if from Markdown to the appropriate format, so the file to change is packaging/manpage.md
  • Write a correct copyright file. Still a bit confused as the adapter is GPL3 and CalculiX seems to be GPL2, and while the copyright mechanism allows different files to have different licenses, the binary is a mix of both. Insights appreciated !
    FIXED : CalculiX says "distribute with GPL2 or above" which solves the problem. GPL3 it is.
  • Currently, the script builds with preCICE 2.3 but the configuration of the package requires >= 2.2, which is inconsistent. I don't know in what direction to settle. Should we link to an outdated preCICE version, or force people to upgrade ?
    FIX : Package indicates 2.0.0 is enough.

Things to do or think about in the long run :

  • The script is v2.17-specific, but could be made more flexible and build for different versions etc.
  • The package has version "2.17-1" and it's hardcoded, I don't know if we should number the versions according to CCX or to preCICE.
  • I know the preCICE packages has security stuff like a sha256 key, but I have zero knowledge about it, so currently the package doesn't feature this.
  • Building the adapter with PaStiX is much harder, and I spent too many hours trying to do it automatically. (For some reasons, I miss some CUDA related dependencies on Github Actions despite installing the required package, though I have no issues with my own VMs). Furthermore, PaStiX requires a lot of hand-built libraries (with specific flags and so on), so integrating it with the Ubuntu ecosystem is impossible, and we'd have to package the dependencies too, and... Well, it looks like a nightmare.
  • Once the packages are made, we still have to release them.

@fsimonis
Copy link
Member

The following is a good read regarding the license compatibility: https://www.gnu.org/licenses/license-compatibility.html

CalculiX is licensed under the GPLv2 license, which requires any redistributed versions to be licensed under the same license. So, we have an issue.

@boris-martin
Copy link
Collaborator Author

The following is a good read regarding the license compatibility: https://www.gnu.org/licenses/license-compatibility.html

CalculiX is licensed under the GPLv2 license, which requires any redistributed versions to be licensed under the same license. So, we have an issue.

Can't we simply have the adapter be GPL2 instead of GPL3 ? That looks like the best way to remove headaches to me. Unless we can't because we'd need the authorization of all people who contributed to the adapter or something like this ?

Copy link
Member

@MakisH MakisH left a comment

Choose a reason for hiding this comment

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

Very good so far! I already commented on tiny details, thus the amount of comments. Let's have another review iteration later, as now I only looked at the code.

Makefile Outdated Show resolved Hide resolved
.github/workflows/ubuntu_build.yml Outdated Show resolved Hide resolved
.github/workflows/ubuntu_build.yml Outdated Show resolved Hide resolved
.github/workflows/ubuntu_build.yml Outdated Show resolved Hide resolved
.github/workflows/ubuntu_build.yml Outdated Show resolved Hide resolved
packaging/make_deb.sh Outdated Show resolved Hide resolved
packaging/make_deb.sh Outdated Show resolved Hide resolved
packaging/manpage.md Outdated Show resolved Hide resolved
packaging/manpage.md Outdated Show resolved Hide resolved
packaging/manpage.md Outdated Show resolved Hide resolved
@MakisH
Copy link
Member

MakisH commented Nov 10, 2021

Currently runs on Ubuntu 18.04 and 20.04 (The 2 latest LTS versions). It should work on other versions but I couldn't get Github to run on 21.04, and it stalled the entire workflow forever so I removed it.

Where did it hang with 21.04? Did you get any error messages?

Usage : download the corresponding artifact and sudo apt install [packagefile]. This requires an installation of preCICE through the provided packages (i.e. this package won't work with a custom build of preCICE, as it explicitly requires the libprecice package)

We would then also need to add this information to the adapter documentation.

The makefile has been updated to be similar (just replaced 2.16 by 2.17 everywhere) to the one of the master branch instead of the current v2.17, which is outdated and painful to use, especially within scripts.

Can we just merge this change everywhere needed, please? :-) This is anyway what we have currently documented.

* The package has version "2.17-1" and it's hardcoded, I don't know if we should number the versions according to CCX or to preCICE.

We need a versioning scheme here and we should put some thought on it. Since the adapter works with only specific versions of CalculiX, but any preCICE v2.x, I would suggest something like calculix-precice2-2.17-1.

* I know the preCICE packages has security stuff like a sha256 key, but I have zero knowledge about it, so currently the package doesn't feature this.

You can generate such a hash very easily, with sha256sum <file>. This is mainly to certify that the package has not been modified.

* Building the adapter with PaStiX is much harder, and I spent too many hours trying to do it automatically. (For some reasons, I miss some CUDA related dependencies on Github Actions despite installing the required package, though I have no issues with my own VMs). Furthermore, PaStiX requires a lot of hand-built libraries (with specific flags and so on), so integrating it with the Ubuntu ecosystem is impossible, and we'd have to package the dependencies too, and... Well, it looks like a nightmare.

Let's forget about PaStiX in the Debian package right now. This PR is already a significant step ahead.

* Once the packages are made, we still have to release them.

We should eventually also start making releases here, similarly to other adapters. This would be much better than the current branching model.

Can't we simply have the adapter be GPL2 instead of GPL3 ? That looks like the best way to remove headaches to me. Unless we can't because we'd need the authorization of all people who contributed to the adapter or something like this ?

As we already discussed on Gitter, maybe this is indeed what we should do. Reaching out to all contributors should be still doable. But let's make sure first that this is the right thing to do.

boris-martin and others added 6 commits November 10, 2021 14:18
Co-authored-by: Gerasimos Chourdakis <chourdak@in.tum.de>
Co-authored-by: Gerasimos Chourdakis <chourdak@in.tum.de>
Co-authored-by: Gerasimos Chourdakis <chourdak@in.tum.de>
Co-authored-by: Gerasimos Chourdakis <chourdak@in.tum.de>
Co-authored-by: Gerasimos Chourdakis <chourdak@in.tum.de>
Co-authored-by: Gerasimos Chourdakis <chourdak@in.tum.de>
@boris-martin
Copy link
Collaborator Author

Currently runs on Ubuntu 18.04 and 20.04 (The 2 latest LTS versions). It should work on other versions but I couldn't get Github to run on 21.04, and it stalled the entire workflow forever so I removed it.

Where did it hang with 21.04? Did you get any error messages?

No errors, just infinite waiting. Maybe they have already switched everything to 21.10 ?

Usage : download the corresponding artifact and sudo apt install [packagefile]. This requires an installation of preCICE through the provided packages (i.e. this package won't work with a custom build of preCICE, as it explicitly requires the libprecice package)

We would then also need to add this information to the adapter documentation.

Yes, I was thinking of waiting for the official release.

The makefile has been updated to be similar (just replaced 2.16 by 2.17 everywhere) to the one of the master branch instead of the current v2.17, which is outdated and painful to use, especially within scripts.

Can we just merge this change everywhere needed, please? :-) This is anyway what we have currently documented.

I'm all in favor, but I'd prefer someone more experienced handled that. (Not really changing the makefile, but 2.17 branch is already 45 commits behind master, so that's slightly messy)

* The package has version "2.17-1" and it's hardcoded, I don't know if we should number the versions according to CCX or to preCICE.

We need a versioning scheme here and we should put some thought on it. Since the adapter works with only specific versions of CalculiX, but any preCICE v2.x, I would suggest something like calculix-precice2-2.17-1.

By the way, I'm not 100% sure what preCICE version to use for the build. If we compile with 2.3 but use only features from <= 2.2, would the adapter work on a computer with preCICE 2.2 installed, for instance ? More generally, should we always build with the most recent version of preCICE 2.x ?

* I know the preCICE packages has security stuff like a sha256 key, but I have zero knowledge about it, so currently the package doesn't feature this.

You can generate such a hash very easily, with sha256sum <file>. This is mainly to certify that the package has not been modified.

So we simply need to provide the output of sha256sum so that people can check ?

* Building the adapter with PaStiX is much harder, and I spent too many hours trying to do it automatically. (For some reasons, I miss some CUDA related dependencies on Github Actions despite installing the required package, though I have no issues with my own VMs). Furthermore, PaStiX requires a lot of hand-built libraries (with specific flags and so on), so integrating it with the Ubuntu ecosystem is impossible, and we'd have to package the dependencies too, and... Well, it looks like a nightmare.

Let's forget about PaStiX in the Debian package right now. This PR is already a significant step ahead.

That seems very reasonable to me.

* Once the packages are made, we still have to release them.

We should eventually also start making releases here, similarly to other adapters. This would be much better than the current branching model.

Even with clean releases, there's still the issue of having different code for each CalculiX version no ?

@MakisH MakisH mentioned this pull request Nov 10, 2021
7 tasks
Co-authored-by: Gerasimos Chourdakis <chourdak@in.tum.de>
@MakisH
Copy link
Member

MakisH commented Nov 10, 2021

No errors, just infinite waiting. Maybe they have already switched everything to 21.10 ?

21.04 is still supported (and I am still using it).

By the way, I'm not 100% sure what preCICE version to use for the build. If we compile with 2.3 but use only features from <= 2.2, would the adapter work on a computer with preCICE 2.2 installed, for instance ? More generally, should we always build with the most recent version of preCICE 2.x ?

We should build/test with the latest version of preCICE. But, because we are linking to a shared library, any preCICE v2.x version will work. (there was even an AdvProg slide on lecture "Build time" regarding exactly this example 😅 )

So we simply need to provide the output of sha256sum so that people can check ?

Exactly. Unless linter suggests more steps.

Even with clean releases, there's still the issue of having different code for each CalculiX version no ?

Yes. But we should have a clear policy on which CalculiX versions we support. I would vote to only support the latest one.

@boris-martin
Copy link
Collaborator Author

No errors, just infinite waiting. Maybe they have already switched everything to 21.10 ?

21.04 is still supported (and I am still using it).

Weird, I should give it another try at some point.

By the way, I'm not 100% sure what preCICE version to use for the build. If we compile with 2.3 but use only features from <= 2.2, would the adapter work on a computer with preCICE 2.2 installed, for instance ? More generally, should we always build with the most recent version of preCICE 2.x ?

We should build/test with the latest version of preCICE. But, because we are linking to a shared library, any preCICE v2.x version will work. (there was even an AdvProg slide on lecture "Build time" regarding exactly this example 😅 )

Woops :D

So we simply need to provide the output of sha256sum so that people can check ?

Exactly. Unless linter suggests more steps.

As far as I remember it didn't complain about the lack of sha256, I was more thinking about how we just publish keys every time there's a new preCICE release.

Even with clean releases, there's still the issue of having different code for each CalculiX version no ?

Yes. But we should have a clear policy on which CalculiX versions we support. I would vote to only support the latest one.

Probably a good idea to add support for 2.18 before dropping the others then !

packaging/manpage.md Outdated Show resolved Hide resolved
@boris-martin
Copy link
Collaborator Author

Slightly unrelated, but following this precice/precice#1132 (comment) I realized it was probably interesting (in the long run) to modify Makefiles to provice a make instal which includes a few things from here, like man pages, copyright and changelog.

@MakisH
Copy link
Member

MakisH commented Nov 10, 2021

Slightly unrelated, but following this precice/precice#1132 (comment) I realized it was probably interesting (in the long run) to modify Makefiles to provice a make instal which includes a few things from here, like man pages, copyright and changelog.

I am not sure if investing on Make would be the right thing to do in that direction. Migrating to CMake could open such possibilities, but I guess it would be complicated to do it in the downstream project (adapter) without already having support from the upstream (CalculiX + dependencies).

@MakisH
Copy link
Member

MakisH commented Nov 10, 2021

Regarding licensing, this is the license of CalculiX, as found in the CalculiX.h of version 2.17:

/*     CALCULIX - A 3-dimensional finite element program                 */
/*              Copyright (C) 1998-2020 Guido Dhondt                     */

/*     This program is free software; you can redistribute it and/or     */
/*     modify it under the terms of the GNU General Public License as    */
/*     published by the Free Software Foundation; either version 2 of    */
/*     the License, or (at your option) any later version.               */

/*     This program is distributed in the hope that it will be useful,   */
/*     but WITHOUT ANY WARRANTY; without even the implied warranty of    */ 
/*     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the      */
/*     GNU General Public License for more details.                      */

/*     You should have received a copy of the GNU General Public License */
/*     along with this program; if not, write to the Free Software       */
/*     Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.         */

Since we have the either version 2 of the License, or (at your option) any later version., I would say we have no problem with GPLv3.

From the FSF article that @fsimonis posted above:

The FSF uses the approach of asking people to release programs under “GNU GPL version N or any later version.” This licensing is compatible with version N, and also with N+1 (because it offers version N+1 as an option). When you combine code under “GPL 3 or later” with code under “GPL 2 or later”, the license of the combination is their intersection, which is “GPL 3 or later”.

The correct license of calculix-precice, as a code derived from CalculiX, and given our additions as GPLv3, is GPLv3.

In our case, we maintain the GPLv2 notice in CalculiX.h, but also provide GPLv3 for our code. I think this is ok but again, I barely understand licensing.

The Debian package is now named "calculix-precice2" and not "calculix-precice". The Github Action workflow was modified to allow more flexible updates.
There is now a variable which helps reduce the number of things to change to upgrade CalculiX version or package version.
@boris-martin boris-martin marked this pull request as ready for review November 21, 2021 01:14
@boris-martin
Copy link
Collaborator Author

boris-martin commented Nov 21, 2021

I did some improvements :

  • I finally modified the copyright file, with GPL3
  • I put preCICE 2.0.0 as prerequisite instead of 2.2
  • I added SHA256 key generation & export
  • I cleaned a few things, especially the workflow file, with an environment variable to have less things to change when we change CalculiX version
  • The package is now named calculix-precice2 instead of calculix-precice

I'm undrafting the PR, though I also plan some adjustements to packagin/README.md

boris-martin and others added 2 commits November 21, 2021 02:19
Co-authored-by: Gerasimos Chourdakis <chourdak@in.tum.de>
packaging/README.md Outdated Show resolved Hide resolved
packaging/README.md Outdated Show resolved Hide resolved
boris-martin and others added 2 commits November 22, 2021 16:15
French typography looks better, but is not appropriate here.

Co-authored-by: Gerasimos Chourdakis <chourdak@in.tum.de>
Removed the Author description (see https://linux.die.net/man/7/man-pages) and added a link to the Github issues for reporting bugs
packaging/manpage.md Outdated Show resolved Hide resolved
Co-authored-by: Gerasimos Chourdakis <chourdak@in.tum.de>
@boris-martin
Copy link
Collaborator Author

@MakisH @KyleDavisSA I don't have the rights to fix the conflict, but it's a trivial one.
After this I think it's ready to merge

Copy link
Member

@MakisH MakisH left a comment

Choose a reason for hiding this comment

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

  • Fixed the conflict.
  • I tried downloading the packages for 20.04 and doing some simple checks. The sha256sum seems to be correct. Lintian is happy apart from the one change suggested here.
  • I have not tried installing the generated packages. Maybe try to install preCICE and the adapter from Debian packages in a clean Ubuntu 20.04 VM before merging?
  • The rest looks good!

packaging/calculix-precice2_2.17-1_amd64/DEBIAN/control Outdated Show resolved Hide resolved
Lintian-conforming: Don't start with an article.

Co-authored-by: Gerasimos Chourdakis <chourdak@in.tum.de>
@boris-martin
Copy link
Collaborator Author

It works on my working Ubuntu VM, worked on my Mint VM before this VM died, and on a clean WSL Ubuntu 20.04. I think tests are conclusive :)

@MakisH MakisH merged commit 5d42fb6 into precice:v2.17 Dec 13, 2021
@uekerman
Copy link
Member

Nice! Please don't forget to also add this to the adapter documentation.

@MakisH
Copy link
Member

MakisH commented Dec 13, 2021

Nice! Please don't forget to also add this to the adapter documentation.

Indeed, but we should probably start making releases first.

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.

5 participants