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

Redo autoconf #26

Merged
merged 13 commits into from
Dec 27, 2023
Merged

Redo autoconf #26

merged 13 commits into from
Dec 27, 2023

Conversation

jedwards4b
Copy link
Contributor

Redo autoconf build system to be more standard.

@jedwards4b jedwards4b self-assigned this Dec 22, 2023
@jedwards4b jedwards4b requested a review from gold2718 December 24, 2023 21:56
@jedwards4b jedwards4b marked this pull request as draft December 24, 2023 21:56
@jedwards4b jedwards4b marked this pull request as ready for review December 24, 2023 21:57
Copy link

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

There seem to be a lot of changes that are unrelated to the stated purpose of this PR (redo autoconf). If that is a correct assessment, it would be helpful to add those items to the PR description or move these changes to a different branch for later inclusion (some seems unfinished, much is commented out).

configure.ac Outdated Show resolved Hide resolved
COPYING Outdated Show resolved Hide resolved
recv.c Outdated Show resolved Hide resolved
send.c Outdated Show resolved Hide resolved
send.c Outdated Show resolved Hide resolved
type.c Outdated Show resolved Hide resolved
type.c Outdated Show resolved Hide resolved
configure Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
aclocal.m4 Outdated Show resolved Hide resolved
@jedwards4b jedwards4b requested a review from gold2718 December 26, 2023 14:25
@jedwards4b
Copy link
Contributor Author

Those changes were removed in the previous PR - this one just needed to be merged to master.

@gold2718
Copy link

Those changes were removed in the previous PR - this one just needed to be merged to master.

Thanks, it can be hard to figure out what the code will look like when the PR is against an old hash.

Copy link

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

Have you tested an install?
I am approving but it would be nice to know what testing has been done (besides make tests which I sure hope was run).

@jedwards4b jedwards4b merged commit c9b6cdc into main Dec 27, 2023
1 check passed
@jedwards4b jedwards4b deleted the redo_autoconf branch December 27, 2023 14:51
@jedwards4b
Copy link
Contributor Author

I have tested on derecho, using spack on derecho and the github action is also installing and running make check. thank you

@@ -0,0 +1,674 @@
GNU GENERAL PUBLIC LICENSE
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of a file called COPYING with the GPL in it? This software is covered by the BSD 3-clause.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was generated by automake and I didn't look at it too closely. I will correct.

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.

3 participants