Draft: Tweak test pipeline conan olaf

QuTech-Delft / qx-simulator

QX Simulator

Other

16 stars 12 forks source link

Draft: Tweak test pipeline conan olaf #139

Closed oschusler closed 1 year ago

pablolh commented 1 year ago

I've identified two remaining problems:

Linux ARM: since the architecture is arm, installation of Bison and Flex via Conan is skipped inside the container. Therefore, no Bison or Flex is present. CMake in tree-gen (which hasn't been updated to Conan) tries to build it and fail.

Suggested solution: install Bison and Flex manually with a workflow step in Linux ARM

MacOS ARM: this is outside of any container, but the self-hosted runner has apparently Bison 2.3 which is too old as per tree-gen's CMake requirements. Building from source fails. Suggested solution: either update the Bison version in the self-hosted runner to 3.0.0+, or lower the requirements in tree-gen's CMake.

rturrado commented 1 year ago

I've identified two remaining problems:

Linux ARM: since the architecture is arm, installation of Bison and Flex via Conan is skipped inside the container. Therefore, no Bison or Flex is present. CMake in tree-gen (which hasn't been updated to Conan) tries to build it and fail.

Suggested solution: install Bison and Flex manually with a workflow step in Linux ARM

MacOS ARM: this is outside of any container, but the self-hosted runner has apparently Bison 2.3 which is too old as per tree-gen's CMake requirements. Building from source fails. Suggested solution: either update the Bison version in the self-hosted runner to 3.0.0+, or lower the requirements in tree-gen's CMake.

If you have a look at the current GitHub for libqasm, starting line 54, but mainly comment from lines 72-74 for libqasm, we install Flex, Bison, and Java for ARM builds, yes. I would go for that solution for self-hosted runners.

rturrado commented 1 year ago

@Pablo, maybe we need to edit the Branch protection rules and remove the ARM64 builds from the Required list.

oschusler commented 1 year ago

I don't really agree that ARM should be removed from the required list. These packages are public facing and should be tested on the systems they are supposed to support. If we remove them from the required list, this means that they are allowed to fail and we can continue without ARM support

rturrado commented 1 year ago

I don't really agree that ARM should be removed from the required list. These packages are public facing and should be tested on the systems they are supposed to support. If we remove them from the required list, this means that they are allowed to fail and we can continue without ARM support

I don't agree either, Olaf. I was meaning: "if you want to merge this quickly, and not wait until we have the self-hosted runners properly set up". That is what we are doing for libqasm at the moment, for example.

Other than that, I am more than 100% in line with your message.