search

jpmorganchase / QOKit

QOKit
https://www.jpmorgan.com/technology/applied-research
Apache License 2.0
51 stars 23 forks source link

Refactor #76

Closed alex124585 closed 4 weeks ago

alex124585 commented 1 month ago

fixed pipeline with MacOS

alex124585 commented 1 month ago

@rsln-s Any update on review?

alex124585 commented 4 weeks ago

Hey @alex124585, apologies for the delay in reviewing this. I see that the main change is the introduction of gcc compiler, which should make the build on MacOS succeed. However, the macOS build is not tested anywhere since the test (e.g., https://github.com/jpmorganchase/QOKit/blob/d624e21b922dd7f3e1873e4dab68b7b3a60af061/tests/test_simulator_build.py) is not present in this branch.

Does this make sense to you or am I missing something? If I'm correct that C simulator build is not tested anywhere in this branch, can you please add tests/test_simulator_build.py from bump-build-version branch. These would show whether the change you implemented in this PR actually fixes the macOS simulator build.

We discussed this in our meeting and agreed make one step at the time. We have many branches which out of sync with main. After pipeline fix I move tests as second step

rsln-s commented 4 weeks ago

But how can I tell if this change resolves the problem with macOS C simulator build if it's not tested anywhere? Can you help me understand how the correctness of the change proposed in this PR is verified?

alex124585 commented 4 weeks ago

But how can I tell if this change resolves the problem with macOS C simulator build if it's not tested anywhere? Can you help me understand how the correctness of the change proposed in this PR is verified?

This change is to fix pipeline for macOS which was due

But how can I tell if this change resolves the problem with macOS C simulator build if it's not tested anywhere? Can you help me understand how the correctness of the change proposed in this PR is verified?

This change is to fix gcc installation on macOS. We discussed this during our meeting last week and were all agreed on that. Have nothing to do with testing for C simulator. To run tests for c simulator 1st it have to be build correctly and point to correct gcc which is correctly installed and configured on macOS.

rsln-s commented 4 weeks ago

I think I still don't understand. The pipeline succeeds even without brew install gcc (see c6c4f21).

It would make it easier for me to review the PR if the title or description were more descriptive than "refactor" or "fixed pipeline with MacOS". For example, it would be helpful if the description stated the problem with the pipeline that is fixed by this PR.

If the goal of this PR is simply to add macOS back to the pipeline, I'm OK with the current diff.