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

Windows CI #6

Merged
merged 26 commits into from
May 18, 2022
Merged

Windows CI #6

merged 26 commits into from
May 18, 2022

Conversation

publixsubfan
Copy link
Member

No description provided.

@publixsubfan publixsubfan mentioned this pull request Apr 20, 2022
2 tasks
@publixsubfan publixsubfan force-pushed the master branch 2 times, most recently from 6ce51e3 to 23c5e5e Compare April 27, 2022 23:30
@tzanio tzanio self-assigned this Apr 29, 2022
@tzanio
Copy link
Member

tzanio commented Apr 29, 2022

Note: this should probably be reviewed together with mfem/mfem#2973

Copy link
Member

@tzanio tzanio left a comment

Choose a reason for hiding this comment

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

Thanks @kanye-quest!

- name: Install Hypre
run: |
echo "Map target to options"
if [[ "${{ inputs.target }}" == "int32" ]]
then
export hypre_options="--disable-fortran";
Copy link
Member

Choose a reason for hiding this comment

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

Why is this removed?

(its purpose is to save a bit of build time as we don't need hypre's Fortran dependencies)

Choose a reason for hiding this comment

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

When I looked into the Hypre documentation some time ago I couldn't find any reference to this option. Are you sure it is still a thing? We had a brief chat with @v-dobrev about it, and I believe he was also thinking that maybe we should remove it.

Copy link
Member

Choose a reason for hiding this comment

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

It is indeed been many years since this was added, but I still see it here: https://github.com/hypre-space/hypre/blob/master/src/configure#L1514

Maybe it is not doing anything anymore? Part of the reason to use it was that otherwise one needed a Fortran compiler, which may not be available by default e.g. on a Mac.

If that's not an issue anymore, I am OK with removing it.

build-hypre/action.yml Show resolved Hide resolved
Copy link
Collaborator

@adrienbernede adrienbernede left a comment

Choose a reason for hiding this comment

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

I need some answers :)

build-hypre/action.yml Show resolved Hide resolved
build-mfem/action.yml Show resolved Hide resolved
build-mfem/action.yml Show resolved Hide resolved
build-mfem/action.yml Show resolved Hide resolved
Copy link
Collaborator

@adrienbernede adrienbernede left a comment

Choose a reason for hiding this comment

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

My main concern is the design comment. @kanye-quest if you agree with that, then maybe we can move forward and make the change, this has no impact on functionality.

build-hypre/action.yml Show resolved Hide resolved
@tzanio
Copy link
Member

tzanio commented May 8, 2022

@adrienbernede, can you take another look?

Copy link
Collaborator

@adrienbernede adrienbernede left a comment

Choose a reason for hiding this comment

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

Looks good to me. Feel free to accept or reject the suggested comment.

Co-authored-by: Adrien Bernede <[email protected]>
@tzanio tzanio changed the base branch from master to v2.2 May 18, 2022 01:57
@tzanio
Copy link
Member

tzanio commented May 18, 2022

Thanks @kanye-quest !

@tzanio tzanio merged commit c205ed5 into mfem:v2.2 May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants