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.

Sign up for GitHub

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

Jump to bottom

Move AIIDALAB_VERSION and AIIDALAB_HOME_VERSION to build.json #327

Merged
merged 12 commits into from
Nov 28, 2022
Merged

Move AIIDALAB_VERSION and AIIDALAB_HOME_VERSION to build.json #327

merged 12 commits into from
Nov 28, 2022

Conversation

danielhollas
Copy link
Contributor

Also removed their defaults in Dockerfile, same fix as done in #311 for AIIDA_VERSION to reduce duplication.

@danielhollas danielhollas self-assigned this Nov 24, 2022
@danielhollas danielhollas removed the request for review from unkcpz November 24, 2022 15:24
@danielhollas danielhollas marked this pull request as draft November 24, 2022 15:24
@danielhollas danielhollas mentioned this pull request Nov 24, 2022
Merged
@danielhollas danielhollas added the blocked This issue/PR is blocked by another issue/PR. label Nov 24, 2022
@unkcpz
Copy link
Member

unkcpz commented Nov 26, 2022 •
edited
Loading

Hi @danielhollas, as I proposed at #332, the test can be simplified without checking the variant parameter.

@danielhollas danielhollas removed the blocked This issue/PR is blocked by another issue/PR. label Nov 28, 2022
@danielhollas danielhollas marked this pull request as ready for review November 28, 2022 12:32
@danielhollas
Copy link
Contributor Author

Hi @unkcpz,

this one is ready to go as well. I have bumped the aiidalab-home version and the version check now passes.

Hi @danielhollas, as I proposed at #332, the test can be simplified without checking the variant parameter.

I have kept the tests as is for now, I think we need more discussion about which images to test.

unkcpz
unkcpz previously approved these changes Nov 28, 2022
View reviewed changes
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

@danielhollas Thanks! Looks all good, just bump the aiidalab package version and I'll merge it.

build.json Outdated Show resolved Hide resolved
@danielhollas
Copy link
Contributor Author

@unkcpz thanks. I've bumped the version. I think once the build finishes we should test it locally before merging, and make sure the packages are indeed getting installed to ~/.local/

@unkcpz
Copy link
Member

unkcpz commented Nov 28, 2022 •
edited
Loading

Seems we need also release conda-fore of aiidalab, or we need to change the install method to pip.

EDIT: I'll do it

@danielhollas
Copy link
Contributor Author

Ugh, right. I guess that's another strike against conda and plus for using pip to install aiidalab and aiida-core.

@danielhollas
Copy link
Contributor Author

Ugh, right. I guess that's another strike against conda and plus for using pip to install aiidalab and aiida-core.

unkcpz
unkcpz approved these changes Nov 28, 2022
View reviewed changes
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Tested with installing AWB, all good! thanks @danielhollas

@unkcpz unkcpz merged commit 749c2c0 into main Nov 28, 2022
@unkcpz unkcpz deleted the build-json branch November 28, 2022 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@unkcpz unkcpz unkcpz approved these changes

Assignees

@danielhollas danielhollas

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@danielhollas @unkcpz