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

Update Cymetric CI #188

Merged
merged 29 commits into from
Nov 29, 2023
Merged

Update Cymetric CI #188

merged 29 commits into from
Nov 29, 2023

Conversation

bennibbelink
Copy link
Contributor

@bennibbelink bennibbelink marked this pull request as ready for review November 29, 2023 01:10
@bennibbelink bennibbelink requested a review from gonuke November 29, 2023 01:10
@bennibbelink bennibbelink linked an issue Nov 29, 2023 that may be closed by this pull request
@bennibbelink
Copy link
Contributor Author

Also updated the README, setup.py, and added a pyproject.toml to completely address #186

@bennibbelink bennibbelink linked an issue Nov 29, 2023 that may be closed by this pull request
@bennibbelink bennibbelink marked this pull request as draft November 29, 2023 01:33
@bennibbelink bennibbelink marked this pull request as ready for review November 29, 2023 01:43
@bennibbelink
Copy link
Contributor Author

When we eventually push a new cyclus python package, we should add it as a required dependency in pyproject.toml

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Thanks @bennibbelink - this looks like a lot of tedious changes. There are a lot of strange whitespace changes, too.

Looks as though you may be changing some things as I review... but here are a few comments.


- name: Build Cymetric
run: |
apt update && apt install -y python3-pip
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this depend on whether we are apt or conda? Or we should install it a dependency back when cyclus builds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I added another step in the workflow. Eventually in cyclus we should use the python -m pip install method to install cyclus instead of just calling setup.py directly, so we will need pip as a dependency anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've implemented this in a branch of cyclus that I have, along with some updates to the cmake build. I stumbled along some fixes while trying to get cython working, but I can make a separate PR for these general build changes

.github/workflows/build_test_publish.yml Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

One of these days we should make this an action we can reuse across projects.

docker/Dockerfile Outdated Show resolved Hide resolved
tests/test-input.xml Outdated Show resolved Hide resolved
tests/myname Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

more whitespace differences making this hard to review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you click the settings icon you can choose to hide whitespace. This seems to work for me, it is easier to read changes

Copy link
Member

Choose a reason for hiding this comment

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

Yeah - I remembered that... but still confused by all the phantom whitespace changes.

@gonuke gonuke merged commit 77fcf6c into main Nov 29, 2023
9 checks passed
@bennibbelink bennibbelink deleted the convert-ci branch January 29, 2024 15:53
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.

Switch to GitHub Actions for CI Deprecation issues
2 participants