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

Drop obsolete scripts #110

Merged
merged 2 commits into from
Jan 16, 2025
Merged

Drop obsolete scripts #110

merged 2 commits into from
Jan 16, 2025

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Jan 16, 2025

More cleanup, getting rid of makefile-based scripts

A couple scripts were to configure the AWS container which we have not used in 5y.
The other is supposed to be for updating the makefiles that we already removed.

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

cm_generate_makefile and generate_makefile invoke CMake in fact.

@dalg24
Copy link
Member Author

dalg24 commented Jan 16, 2025

@lucbv do you want to preserve these scripts?

@lucbv
Copy link
Contributor

lucbv commented Jan 16, 2025

@ndellingwood can you have a look at this?

@lucbv
Copy link
Contributor

lucbv commented Jan 16, 2025

@dalg24 your title says you are dropping obsolete scripts, can you mention somewhere on this PR what the replacement for these are? That would make the review a lot easier

@dalg24 dalg24 force-pushed the drop_obsolete_scripts branch from 7576c25 to 712fda9 Compare January 16, 2025 18:36
@dalg24
Copy link
Member Author

dalg24 commented Jan 16, 2025

@dalg24 your title says you are dropping obsolete scripts, can you mention somewhere on this PR what the replacement for these are? That would make the review a lot easier

Withdrew the Kernels part and updated the description

@lucbv
Copy link
Contributor

lucbv commented Jan 16, 2025

Then sure no problem from my side

@ndellingwood
Copy link
Contributor

ndellingwood commented Jan 16, 2025

cm_generate_makefile and generate_makefile invoke CMake in fact.

@masterleinad

Yes, I never renamed the scripts to reflect they now internally use cmake, the naming is a carry-over from pre-cmake build system support (I think 3.0) and their usage in Jenkins builds

From a glance, the scripts modified in this PR should still be functional and I would vote against their removal without some replacement directly calling cmake. In terms of the misleading naming of the {cm_}generate_makefile scripts, and their location in the resp. repos, I am overdue to do the following and will take care of this:

  • Rename and update
  • Move the scripts from the base directory to the scripts directory
  • Update nightly jobs accordingly that still use the scripts

@dalg24 @lucbv let me know if you'd prefer to have the scripts in the PR updated after rename+reorg of generate_makefile etc. (fast and easy update), or if the preference is to have these scripts updated/replaced that directly call cmake (a bit more work)

@dalg24 dalg24 merged commit fd4852c into kokkos:main Jan 16, 2025
3 of 4 checks passed
@dalg24 dalg24 deleted the drop_obsolete_scripts branch January 16, 2025 19:02
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.

4 participants