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

adapt Libint library for Windows #270

Merged
merged 5 commits into from
Oct 12, 2023
Merged

Conversation

loriab
Copy link
Collaborator

@loriab loriab commented Aug 30, 2023

(currently atop #269)

(in decreasing order of importance)

  • adapt library and Python bindings build system for Windows. This mostly involves adding the error handling flags and math defines for constants. Adapt various headers, types, and filesystem and memory commands for Windows in library source and headers.
  • note that the adaptation of the compiler/generator source for Windows is in cmake harness, 2023 edition #259, not here. But though that builds and generates, and the resulting library compiles, it does not produce correct results. This PR confines Windows to building and running Linux-generated library src.
  • OLD: Windows doesn't like files with * in the name, so 6-311g**.g94, 6-31g**, 6-31g*, and 6-31G* are renamed in the library and in tests to 6-311gss.g94, etc. If more Pople basis sets are planned, should instead do something like https://github.com/psi4/psi4/blob/1a971f184783b3e5c02e0055f508918d6fb56355/psi4/driver/qcdb/basislist.py#L162-L167, so the first becomes 6-311g_d_p_.g94
  • EDIT: Windows doesn't like files with * in the name (shows as unprintable, ), so 6-311g**.g94, 6-31g**, 6-31g*, and 6-31G* are symlinked in the repo to 6-311gss.g94, etc. For all OS, either "s" or "*" in the basis string name accesses the basis set. For Windows, only the "s" g94 files are installed, while for other OSes, both variants are installed. An extra test is added to unit/basis to show both work.
  • changed the INSTALL_INTERFACE:DATADIR so that it is relocatable when _IMPORT_PREFIX defined in libint2-config.cmake. The current syntax only works for Unix; conda is handling the Windows case that's painful to iteratively debug.
  • add Windows build (clang-cl compiler atop MSVC) to GHA. Add library unit tests to second GHA stage to ensure Intel and Windows are passing those, too, not just the Hartree--Fock example.
  • updated GHA syntax and actions to fix all the deprecation warnings.
  • added tentative changelog entries for recent and pending PRs.

@loriab loriab changed the title windows WIP adapt Libint library for Windows Sep 3, 2023
@loriab loriab marked this pull request as ready for review September 3, 2023 05:11
@loriab
Copy link
Collaborator Author

loriab commented Sep 18, 2023

@evaleev , don't worry too much about the GHA fails on master. I ran into the "Repo" stage skipping once before and tried to make the "Export" stage skip with https://github.com/evaleev/libint/blob/master/.github/workflows/cmake.yml#L165 (can't run b/c no tarball produced by "Repo" stage), but apparently that doesn't work. I'll try something else.

@loriab
Copy link
Collaborator Author

loriab commented Sep 18, 2023

Ok, this PR is rebased to master, and GHA checks out. the GHA is likely to complete successfully.

@loriab loriab requested a review from evaleev September 18, 2023 02:31
@evaleev
Copy link
Owner

evaleev commented Sep 18, 2023

@loriab what's the issue with * in filenames on Windows? My concern with renamings is that existing tests/inputs will stop working ... would it be sufficient to provide symlinks to the "new" names and in Basis constructor add Windows-specific logic to change * to s in basis set names?

@loriab
Copy link
Collaborator Author

loriab commented Sep 18, 2023

I don't recall the actual error with *, only that it was reproducible. Sure, I think symlinks and some constructor logic would work. I'll work on that.

@loriab loriab force-pushed the windowsforeal branch 2 times, most recently from b47208d to 607fdb4 Compare September 18, 2023 16:55
@loriab
Copy link
Collaborator Author

loriab commented Sep 18, 2023

From the install below, it looks like Windows plain wasn't coping with the star character. I've now modified it so the star files don't get installed for Windows. Note that for all OSes, while the *->s basis files are symlinks in the repo, by the time they're exported to tarball, they are duplicate files.

2023-09-18T16:00:31.2068989Z -- Installing: D:/a/libint/libint/installed/share/libint/2.8.0/basis
2023-09-18T16:00:31.2069645Z -- Installing: D:/a/libint/libint/installed/share/libint/2.8.0/basis/3-21g.g94
2023-09-18T16:00:31.2070491Z -- Installing: D:/a/libint/libint/installed/share/libint/2.8.0/basis/6-311gss.g94
2023-09-18T16:00:31.3381459Z -- Installing: D:/a/libint/libint/installed/share/libint/2.8.0/basis/6-311g.g94
2023-09-18T16:00:31.3974234Z -- Installing: D:/a/libint/libint/installed/share/libint/2.8.0/basis/6-31g.g94
2023-09-18T16:00:31.4890708Z -- Installing: D:/a/libint/libint/installed/share/libint/2.8.0/basis/6-31gs.g94
2023-09-18T16:00:31.4895908Z -- Installing: D:/a/libint/libint/installed/share/libint/2.8.0/basis/6-31gss.g94
2023-09-18T16:00:31.4916508Z -- Installing: D:/a/libint/libint/installed/share/libint/2.8.0/basis/6-31G.g94
2023-09-18T16:00:31.4917618Z -- Installing: D:/a/libint/libint/installed/share/libint/2.8.0/basis/6-31g.g94
2023

@loriab
Copy link
Collaborator Author

loriab commented Sep 18, 2023

Ok, full GHA passing again. The REGEX ... EXCLUDE lines that work correctly in local Linux testing don't actually prevent the star files from installing on Windows, but I suppose the extra files aren't doing any harm. Tests are passing on all OS w/o the earlier basis name edits.

@loriab
Copy link
Collaborator Author

loriab commented Sep 18, 2023

The below shows some promise (on Linux) for filtering out the star files. I'll try it out when rebasing 271 if there isn't another GHA run before then.

    install(DIRECTORY ${PROJECT_SOURCE_DIR}/lib/basis
        COMPONENT libint2
        DESTINATION "${LIBINT2_INSTALL_DATADIR}"
        FILES_MATCHING REGEX "[A-Za-z0-9\\)].g94"
        )

@loriab
Copy link
Collaborator Author

loriab commented Sep 19, 2023

Install working now -- description in PR frontmatter.

@loriab
Copy link
Collaborator Author

loriab commented Sep 29, 2023

I think this is ready for consideration. After this is merged, there's a couple extra changes I can make to #271 (now that SH ordering isn't a choice at generation-time for multipoles) while doing its rebase. I'm glad to make more changes here, too.

Copy link
Owner

@evaleev evaleev left a comment

Choose a reason for hiding this comment

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

thank you @loriab !

@evaleev evaleev merged commit ca32e0c into evaleev:master Oct 12, 2023
@loriab
Copy link
Collaborator Author

loriab commented Oct 12, 2023

thanks! I'll get #271 prepared.

@loriab loriab deleted the windowsforeal branch October 12, 2023 14:31
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.

2 participants