Add new conan remote and use pre-built symengine package #217

cqc-alec · 2022-02-14T14:56:34Z

The plan is to have tket components individually versioned, packaged and hosted on tket.jfrog.io in a new tket-conan repository. This should help make tket more modular and extensible, as well as reducing build times.

As a "warm-up", and to save having to build symengine on the CI, I have:

added a workflow that builds symengine (from our own recipe) and uploads the packages to tket-conan, in several configurations (4 platforms x Release/Debug x shared/static);
changed the CI workflows that build tket so that they can just download symengine from the new remote instead of having to build it;
added the necessary credentials as github secrets;
run the workflows, to test them and populate the repository.

The build_symengine workflow is run only for PRs and pushes to develop that change the symengine recipe. In the PR workflow the artefacts are not actually uploaded; in the push workflow they are.

…changes.

…onan.

cqc-melf

Have you thought about making the build symengine workflow required on the ci?

Have you run the symengine workflow somewhere?

cqc-melf · 2022-02-14T15:32:50Z

README.md

@@ -135,7 +147,9 @@ The `symengine` dependency is built from a local conan recipe. Run:
 conan create --profile=tket recipes/symengine
 ```

-to build it.
+to build it. If you are using a conan configuration supported by the CI, this is


From my understanding this only works for specific compiler versions? If that is a case should we add them here?

Yes that's right. There is a list above under "Build tools". Do you think that is enough? Maybe we should add a reference to that section here.

I have seen you added that, thank you!

cqc-alec · 2022-02-14T15:55:46Z

Have you thought about making the build symengine workflow required on the ci?

Have you run the symengine workflow somewhere?

Yes, I have: here and (after fixing an error in the Windows section) here.

If you add the new conan remote to your local setup you can then do:

conan search -r=tket-conan symengine/0.8.1.1@tket/stable

to see the list of packages there.

cqc-alec · 2022-02-14T16:06:57Z

Have you thought about making the build symengine workflow required on the ci?

Yes, I will probably do that.

I forgot that conan data is cached on MacOS builds.

cqc-melf · 2022-02-14T17:06:14Z

Have you thought about making the build symengine workflow required on the ci?
Have you run the symengine workflow somewhere?

Yes, I have: here and (after fixing an error in the Windows section) here.

If you add the new conan remote to your local setup you can then do:
conan search -r=tket-conan symengine/0.8.1.1@tket/stable
to see the list of packages there.

Thank you, that was helpful in the understanding.

cqc-melf

A few more comments, and one questions:

At which point are you making sure that the symengine compilation is run before the tket compilation in case of changes to symengine?

cqc-melf · 2022-02-14T17:06:11Z

.github/workflows/build_and_test.yml

@@ -54,10 +55,10 @@ jobs:
        ${conan_cmd} profile update settings.compiler.libcxx=libstdc++11 tket
        ${conan_cmd} profile update options.tket:shared=True tket
        echo "CONAN_CMD=${conan_cmd}" >> $GITHUB_ENV
+    - name: add remote
+      run: ${CONAN_CMD} remote add tket-conan https://tket.jfrog.io/artifactory/api/conan/tket-conan


why is this not forced?

It is only on the MacOS platforms that we cache the conan data. (On macos by explicitly caching it, on macos-m1 just by not clearing it since it's the same machine each time.) This is because they need to rebuild (some of) boost so it saves a significant amount to time. On the other platforms there is no point.

cqc-melf · 2022-02-14T17:06:34Z

.github/workflows/build_and_test.yml

@@ -366,14 +370,14 @@ jobs:
        conan profile update options.tket:shared=True tket
        $conan_cmd = (gcm conan).Path
        echo "CONAN_CMD=${conan_cmd}" >> $GITHUB_ENV
+    - name: add remote
+      run: conan remote add tket-conan https://tket.jfrog.io/artifactory/api/conan/tket-conan


cqc-melf · 2022-02-14T17:13:34Z

.github/workflows/build_symengine.yml

+on:
+  push:
+    branches:
+      - develop


I am not sure how this interacts with the possibilities to do hot fixes to only have this run on develop. If there is no different version for the master branch this could create dependencies after changes in symengine that we are not able to handle with this setup. Considering that it is quiet unlikely that this will happen at all, I would be fine with merging this as it is, but I wanted to mention it.

Yes that is a fair point, we could run this on PRs and pushes to main in exactly the same way. The downside is that then for a normal release when we do a PR or push to main it would build the package unnecessarily (this won't cause the build to fail, however, it just won't upload anything).
Given that we do hotfixes for pytket very rarely, I think I'm happy to keep develop as the single place where this is tracked. If we do need to do a hotfix that changes the symengine recipe, we can do a PR to develop with the new recipe (and new version number -- builds of tket on develop will continue to use the old version), followed by the hotfix.

Okay, I agree.

cqc-melf · 2022-02-14T17:15:10Z

.github/workflows/pytket_docs.yml

-    - name: Build symengine
-      run: conan create --profile=tket recipes/symengine
+    - name: add remote
+      run: conan remote add tket-conan https://tket.jfrog.io/artifactory/api/conan/tket-conan


cqc-melf · 2022-02-14T17:15:21Z

.github/workflows/release.yml

@@ -160,14 +166,14 @@ jobs:
        conan profile update options.tket:shared=True tket
        $conan_cmd = (gcm conan).Path
        echo "CONAN_CMD=${conan_cmd}" >> $GITHUB_ENV
+    - name: add remote
+      run: conan remote add tket-conan https://tket.jfrog.io/artifactory/api/conan/tket-conan


cqc-alec · 2022-02-14T17:40:46Z

At which point are you making sure that the symengine compilation is run before the tket compilation in case of changes to symengine?

Sorry I missed this. We are not -- so we would have to first update the symengine recipe in a PR, and then do tket updates that depend on it another PR.

cqc-melf · 2022-02-14T18:15:54Z

At which point are you making sure that the symengine compilation is run before the tket compilation in case of changes to symengine?

Sorry I missed this. We are not -- so we would have to first update the symengine recipe in a PR, and then do tket updates that depend on it another PR.

Oh, that sounds a little bit complicated. Is that documented somewhere?

cqc-alec · 2022-02-14T20:36:36Z

At which point are you making sure that the symengine compilation is run before the tket compilation in case of changes to symengine?

Sorry I missed this. We are not -- so we would have to first update the symengine recipe in a PR, and then do tket updates that depend on it another PR.

Oh, that sounds a little bit complicated. Is that documented somewhere?

In practice I don't think this will be an issue. It's good practice to separate the PRs anyway, and as long as we remember that tket must use a version of symengine that exists on the repo it shouldn't cause confusion. But I will add a comment in the tket conanfile to make this clear.

cqc-alec added 4 commits February 14, 2022 12:58

Build and upload symengine packages on push to develop if the recipe …

e49e1a7

…changes.

Modify tket recipe and workflows to use symengine package from tket-c…

d4dc359

…onan.

Update README.

6f51691

Add note to README about (not) building symengine.

e67df5c

cqc-alec requested a review from cqc-melf February 14, 2022 14:56

cqc-melf reviewed Feb 14, 2022

View reviewed changes

Clarify comment about supported configurations in README.

5855a74

Force add remote.

abe9967

I forgot that conan data is cached on MacOS builds.

cqc-melf reviewed Feb 14, 2022

View reviewed changes

cqc-alec requested a review from cqc-melf February 14, 2022 17:36

cqc-melf previously approved these changes Feb 14, 2022

View reviewed changes

Add comment about symengine remote in tket conanfile.

7f2f355

cqc-alec dismissed cqc-melf’s stale review via 7f2f355 February 14, 2022 20:42

cqc-alec requested a review from cqc-melf February 14, 2022 20:43

cqc-melf approved these changes Feb 14, 2022

View reviewed changes

cqc-alec merged commit 87a4eff into develop Feb 15, 2022

cqc-alec deleted the feature/package-symengine branch February 15, 2022 07:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new conan remote and use pre-built symengine package #217

Add new conan remote and use pre-built symengine package #217

cqc-alec commented Feb 14, 2022

cqc-melf left a comment

cqc-melf Feb 14, 2022

cqc-alec Feb 14, 2022

cqc-melf Feb 14, 2022

cqc-alec commented Feb 14, 2022 •

edited

Loading

cqc-alec commented Feb 14, 2022

cqc-melf commented Feb 14, 2022

cqc-melf left a comment

cqc-melf Feb 14, 2022

cqc-alec Feb 14, 2022

cqc-melf Feb 14, 2022

cqc-alec Feb 14, 2022

cqc-melf Feb 14, 2022

cqc-alec Feb 14, 2022

cqc-melf Feb 14, 2022

cqc-melf Feb 14, 2022

cqc-alec Feb 14, 2022

cqc-melf Feb 14, 2022

cqc-alec Feb 14, 2022

cqc-alec commented Feb 14, 2022

cqc-melf commented Feb 14, 2022

cqc-alec commented Feb 14, 2022 •

edited

Loading

Add new conan remote and use pre-built symengine package #217

Add new conan remote and use pre-built symengine package #217

Conversation

cqc-alec commented Feb 14, 2022

cqc-melf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cqc-alec commented Feb 14, 2022 • edited Loading

cqc-alec commented Feb 14, 2022

cqc-melf commented Feb 14, 2022

cqc-melf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cqc-alec commented Feb 14, 2022

cqc-melf commented Feb 14, 2022

cqc-alec commented Feb 14, 2022 • edited Loading

cqc-alec commented Feb 14, 2022 •

edited

Loading

cqc-alec commented Feb 14, 2022 •

edited

Loading