search

E4S-Project / e4s

E4S for Spack
https://e4s.readthedocs.io
MIT License
30 stars 13 forks source link

Fix: Ninja Repo Link #48

Closed ax3l closed 2 years ago

ax3l commented 2 years ago

The previous link was pointing to an unrelated project.

shahzebsiddiqui commented 2 years ago

Thanks @ax3l I recall this discrepancy I'll defer this to @eugeneswalker

shahzebsiddiqui commented 2 years ago

@eugeneswalker @wspear can you review this MR

shahzebsiddiqui commented 2 years ago

Perhaps @maherou @sameershende can comment on this. Looking at this repo https://github.com/PRUNERS/NINJA its not been updated since 2017 while https://github.com/ninja-build/ninja is in active development . According to spack package https://github.com/spack/spack/blob/develop/var/spack/repos/builtin/packages/ninja/package.py we are installing https://github.com/ninja-build/ninja so suggestion proposed by @ax3l seems reasonable. Atleast from spack perspective we are not installing NINJA from PRUNERs org.

shahzebsiddiqui commented 2 years ago

@wspear that link you shared looks like it was updated in 2017 i think it is bit out of date, i think the spack package makes it clear what we are installing so i think this link should be fixed in E4S docs.

    homepage = "https://ninja-build.org/"
    url      = "https://github.com/ninja-build/ninja/archive/v1.7.2.tar.gz"
    git      = "https://github.com/ninja-build/ninja.git"
wspear commented 2 years ago

Pruners ninja is an ECP-relevant project. The ninja build system is not. I couldn't find any E4S/ECP projects that depend on the ninja build system (based on the results of spack find -dx | grep ninja in an E4S environment). I believe any instances where ninja shows up in E4S instead of pruners-ninja are a result of name confusion and should be corrected. This is not the only case of a complication arising from the default spack package name differing from the product name.

eugeneswalker commented 2 years ago

Pruners ninja is an ECP-relevant project. The ninja build system is not.

This is correct. There is confusion because it is ninja that is being installed as part of E4S Spack environment: https://github.com/E4S-Project/e4s/blob/master/environments/22.02/spack-x86_64.yaml#L119

It sounds like it should be pruners-ninja that is listed in the Spack environment, not ninja.

See Spack package pruners-ninja here https://github.com/spack/spack/tree/develop/var/spack/repos/builtin/packages/pruners-ninja

If we can agree about this, we should keep the current link as is (i.e. do not merge this PR) and we should instead replace ninja with pruners-ninja in E4S Spack environment.

What do you all think?

eugeneswalker commented 2 years ago

I think the way forward here is to change ninja to pruners-ninja in the root spec list for the upcoming E4S 22.05 release. If anyone thinks this is not right let us discuss here.