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

sartre: new package #32713

Merged
merged 7 commits into from
Mar 1, 2023
Merged

sartre: new package #32713

merged 7 commits into from
Mar 1, 2023

Conversation

wdconinc
Copy link
Contributor

@wdconinc wdconinc commented Sep 19, 2022

Sartre is an event generator for exclusive diffractive vector meson production and DVCS in ep and eA collisions based on the dipole model.

Notable dependencies are cuba.

TODO:

  • This version is still forcing the use of the vendored package. Discussion is ongoing with the developer.

[Sartre](https://sartre.hepforge.org) is an event generator for exclusive diffractive vector meson production and DVCS in ep and eA collisions based on the dipole model.

Notable dependencies are [`cuba`](spack#32510), but this version is still forcing the use of the vendored package.
@bernhardkaindl
Copy link
Contributor

@wdconinc Just notifying you that about two months passed since your last comment. You wrote:

This version is still forcing the use of the vendored package. Discussion is ongoing with the developer.

@alalazo
Copy link
Member

alalazo commented Feb 21, 2023

What is the status of this PR?

@wdconinc
Copy link
Contributor Author

Discussion is ongoing with the developer.

This is still not resolved (not gotten an answer). I'll wrap this up today without developer input. I'll just live with the vendored dependency or filter_file it out.

@wdconinc
Copy link
Contributor Author

Things I would have wished the developers could address (but they haven't):

  • /usr/bin/tar: Ignoring unknown extended header keyword 'LIBARCHIVE.xattr.com.apple.lastuseddate#PS' (stuff included in tar archive that is not really supposed to be there; though maybe spack could be less loud about this too, tar --warning=no-unknown-keyword hides this),
  • cuba is forcibly used as vendored dependency in a way that's hard to simply filter_file out,
  • the CMakeLists.txt installs in <prefix>/sartre, which I did filter_file out.

@wdconinc wdconinc marked this pull request as ready for review February 22, 2023 01:23
@wdconinc wdconinc requested a review from alalazo February 22, 2023 01:24
@wdconinc
Copy link
Contributor Author

@spackbot fix style

@spackbot-app
Copy link

spackbot-app bot commented Feb 22, 2023

Let me see if I can fix that for you!

@spackbot-app
Copy link

spackbot-app bot commented Feb 22, 2023

I was able to run spack style --fix for you!

spack style --fix
==> Running style checks on spack
  selected: isort, black, flake8, mypy
==> Modified files
  var/spack/repos/builtin/packages/sartre/package.py
==> Running isort checks
  isort checks were clean
==> Running black checks
reformatted var/spack/repos/builtin/packages/sartre/package.py
All done! ✨ 🍰 ✨
1 file reformatted.
  black checks were clean
==> Running flake8 checks
  flake8 checks were clean
==> Running mypy checks
Success: no issues found in 576 source files
  mypy checks were clean
==> spack style checks were clean
Keep in mind that I cannot fix your flake8 or mypy errors, so if you have any you'll need to fix them and update the pull request. If I was able to push to your branch, if you make further changes you will need to pull from your updated branch before pushing again.

I've updated the branch with style fixes.

But it looks like I'm not able to push to your branch. 😭️ Did you check maintainer can edit when you opened the PR?

@alalazo alalazo merged commit dd854e8 into spack:develop Mar 1, 2023
@wdconinc wdconinc deleted the new-package-sartre branch March 2, 2023 00:56
koysean pushed a commit to koysean/spack that referenced this pull request Mar 7, 2023
jmcarcell pushed a commit to key4hep/spack that referenced this pull request Apr 13, 2023
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.

3 participants