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

Add hepmcmerger #647

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Add hepmcmerger #647

wants to merge 10 commits into from

Conversation

kkauder
Copy link

@kkauder kkauder commented Jun 17, 2024

Briefly, what does this PR introduce?

Add a package for https://github.com/eic/HEPMC_Merger

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • [*] New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No

Does this PR change default behavior?

No

@kkauder kkauder requested a review from wdconinc June 17, 2024 18:18
@veprbl veprbl changed the title Addhepmcmerger Add hepmcmerger Jun 17, 2024
Copy link
Contributor

@wdconinc wdconinc left a comment

Choose a reason for hiding this comment

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

Did you test that this actually works? Maybe you didn't submit all changes to this PR since you define cmake_args without defining this package as a CMakePackage.

packages/hepmcmerger/package.py Outdated Show resolved Hide resolved
@kkauder
Copy link
Author

kkauder commented Jun 18, 2024

I couldn't test it, can you tell me how? This is something assembed from looking at existing packages but I'm flying blind

@wdconinc
Copy link
Contributor

I couldn't test it, can you tell me how? This is something assembed from looking at existing packages but I'm flying blind

You can do something like:

docker run --rm -it spack/ubuntu-noble:0.21.0
git clone https://github.com/eic/eic-spack
git -C eic-spack checkout <branch>
spack repo add eic-spack
spack install <pkg>

(This could have typos, on my phone.)

@kkauder
Copy link
Author

kkauder commented Jun 18, 2024

After chewing on this command for three hours, it just failed with the unrelated

 >> 9686    FAILED: tools/clang/lib/ASTMatchers/Dynamic/CMakeFiles/obj.clangDynamicASTMatchers.dir/Re
             gistry.cpp.o
     9687    /opt/spack/lib/spack/env/gcc/g++ -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_M

Is there anything else I can do?

@wdconinc
Copy link
Contributor

Is there anything else I can do?

Not in this PR, apparently. Can you copy the full error message (with more than 'FAILED') and file it on spack?

@wdconinc
Copy link
Contributor

Is there anything else I can do?

Not in this PR, apparently. Can you copy the full error message (with more than 'FAILED') and file it on spack?

And tag me in the issue on spack.

@kkauder
Copy link
Author

kkauder commented Jun 25, 2024

I undid the f-string. It didn't work and this syntax is literally the same throughout the repo according to grep -r DCMAKE_CXX_STANDARD

@wdconinc
Copy link
Contributor

Use single quotes inside the f-string arguments.

@wdconinc
Copy link
Contributor

See last two examples in https://peps.python.org/pep-0498/#escape-sequences for correct f-string syntax.

kkauder and others added 2 commits June 25, 2024 14:17
Co-authored-by: Wouter Deconinck <wdconinc@gmail.com>
Co-authored-by: Wouter Deconinck <wdconinc@gmail.com>
@kkauder
Copy link
Author

kkauder commented Jun 25, 2024

Thanks for all the help, @wdconinc ! Is the fact that the checks pass good enough to try including it in eic-shell?

@wdconinc
Copy link
Contributor

Not at all. The checks don't do anything ;-)

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