Skip to content

Conversation

@MridulS
Copy link
Member

@MridulS MridulS commented Jan 7, 2025

No description provided.

@MridulS
Copy link
Member Author

MridulS commented Jan 10, 2025

Alrighty, this is ready for review. The main change here is that while building the conda package now we will use the packages from conda and while building the wheel we stick to conan to fetch the deps. I also made some changes to the cpp code to make compilers on mac and windows happy. The windows one was complaining about using reserverd keywords like and, or, not. I also made some cosmetic changes to get rid of some warnings and I can remove them from this PR if we don't want to mix those things up with this change.

Another thing that could be controversial is that I am using different tbb versions for conda and pypi. conda has the latest tbb=2022.0.0 but conan does not have the new releases yet. I can sync them up if required.

The last commit has the action build logs for wheels and conda https://github.com/scipp/scipp/actions/runs/12706404682/job/35419301931

@MridulS MridulS marked this pull request as ready for review January 10, 2025 09:58
@SimonHeybrock
Copy link
Member

Another thing that could be controversial is that I am using different tbb versions for conda and pypi. conda has the latest tbb=2022.0.0 but conan does not have the new releases yet. I can sync them up if required.

We should check if this works with Mantid. In the past we had to stick with some older version because Mantid uses it as well.

Copy link
Member

@SimonHeybrock SimonHeybrock left a comment

Choose a reason for hiding this comment

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

Nice work! Minor questions/comments.

@MridulS
Copy link
Member Author

MridulS commented Jan 10, 2025

We should check if this works with Mantid. In the past we had to stick with some older version because Mantid uses it as well.

Hmm, need to find a machine to run the test. I guess running this https://scipp.github.io/scippneutron/user-guide/from-mantid-to-scipp.html should be enough right?

@SimonHeybrock
Copy link
Member

We should check if this works with Mantid. In the past we had to stick with some older version because Mantid uses it as well.

Hmm, need to find a machine to run the test. I guess running this https://scipp.github.io/scippneutron/user-guide/from-mantid-to-scipp.html should be enough right?

Yes. Why can't you use your own machine? The TBB version bump happened across platforms, didn't it?

@MridulS
Copy link
Member Author

MridulS commented Jan 10, 2025

Why can't you use your own machine

mantid on conda doesn't work with Mac ARM?

@MridulS
Copy link
Member Author

MridulS commented Jan 10, 2025

I was able to go through https://scipp.github.io/scippneutron/user-guide/from-mantid-to-scipp.html on a linux machine just fine. I just checked to see if there were any errors pop up in cells. Didn't see anything pop up.

@MridulS MridulS enabled auto-merge (squash) January 10, 2025 15:25
@MridulS MridulS merged commit 2eb2095 into scipp:main Jan 16, 2025
4 checks passed
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