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

Fixed wheel generation #216

Merged
merged 3 commits into from
Aug 14, 2024
Merged

Fixed wheel generation #216

merged 3 commits into from
Aug 14, 2024

Conversation

lajerm
Copy link
Collaborator

@lajerm lajerm commented Aug 8, 2024

No description provided.

mpmdean
mpmdean reviewed Aug 9, 2024
View reviewed changes
Copy link
Contributor

@mpmdean mpmdean left a comment

Choose a reason for hiding this comment

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

I beleive that it is premature to remove all these apt-get installs, as I don't think installing these via conda is working see #217 , which seems to solve the currently failing docs build.

scripts/install-deps.sh Outdated
platform="$(uname)"

if [ "${platform}" == "Linux" ]; then
# sudo apt-get update -y
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# sudo apt-get update -y
sudo apt-get update -y

scripts/install-deps.sh Outdated
libparpack2-dev
platform="$(uname)"

if [ "${platform}" == "Linux" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

As I checked in #217 it seems that commenting these install is what is breaking the generations of the docs.

I know that we wanted to install these via conda, but I don't think that is properly implemented?

scripts/install-deps.sh Outdated

if [ "${platform}" == "Linux" ]; then
# sudo apt-get update -y
# sudo apt-get install -y \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# sudo apt-get install -y \
sudo apt-get install -y \

scripts/install-deps.sh Outdated
if [ "${platform}" == "Linux" ]; then
# sudo apt-get update -y
# sudo apt-get install -y \
# gfortran \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# gfortran \
gfortran \

scripts/install-deps.sh Outdated
# sudo apt-get update -y
# sudo apt-get install -y \
# gfortran \
# openmpi-bin \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# openmpi-bin \
openmpi-bin \

scripts/install-deps.sh Outdated
# gfortran \
# openmpi-bin \
# openmpi-doc \
# libopenmpi-dev \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# libopenmpi-dev \
libopenmpi-dev \

scripts/install-deps.sh Outdated
# openmpi-bin \
# openmpi-doc \
# libopenmpi-dev \
# libopenblas-dev \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# libopenblas-dev \
libopenblas-dev \

scripts/install-deps.sh Outdated
# openmpi-doc \
# libopenmpi-dev \
# libopenblas-dev \
# libarpack2-dev \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# libarpack2-dev \
libarpack2-dev \

scripts/install-deps.sh Outdated
# libopenmpi-dev \
# libopenblas-dev \
# libarpack2-dev \
# libparpack2-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# libparpack2-dev
libparpack2-dev

scripts/install-deps.sh Outdated
# libopenblas-dev \
# libarpack2-dev \
# libparpack2-dev
echo "Skipping system packages installation in favor of conda packages for building."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
echo "Skipping system packages installation in favor of conda packages for building."
# install these packages via conda in the future

@mrakitin mrakitin mentioned this pull request Aug 14, 2024
Closed
mrakitin
mrakitin reviewed Aug 14, 2024
View reviewed changes
Copy link
Member

@mrakitin mrakitin left a comment

Choose a reason for hiding this comment

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

I think it's ready for review.

@mpmdean, if the CI runs pass - feel free to merge! We'll see if there are any issues with publishing docs as they are published only from the master branch.

@mpmdean
Copy link
Contributor

mpmdean commented Aug 14, 2024

Thanks @mrakitin . The tests indeed all passed, so I will go ahead and merge this.

@mpmdean mpmdean merged commit 4b1c2f5 into master Aug 14, 2024
12 checks passed
@mpmdean mpmdean deleted the fix_wheel_name branch August 14, 2024 15:39
@mrakitin
Copy link
Member

I pushed a few commits to fix the docs artifact uploading/downloading business. Now it works:
https://nsls-ii.github.io/edrixs/reference/index.html
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrakitin mrakitin mrakitin left review comments

@mpmdean mpmdean mpmdean left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lajerm @mpmdean @mrakitin