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
nmap: update to depend on python2 for ndiff and zenmap; add 7.93 #34851
Conversation
Hi @dpzmick! I noticed that the following package(s) don't yet have maintainers:
Are you interested in adopting any of these package(s)? If so, simply add the following to the package class: maintainers = ["dpzmick"] If not, could you contact the developers of this package and see if they are interested? You can quickly see who has worked on a package with $ spack blame nmap Thank you for your help! Please don't add maintainers without their consent. You don't have to be a Spack expert or package developer in order to be a "maintainer," it just gives us a list of users willing to review PRs or debug issues relating to this package. A package can have multiple maintainers; just add a list of GitHub handles of anyone who wants to volunteer. |
As you note, the package audits don't like use of python2. Spack deprecated Python2 in v0.19 and plans to remove its support.
|
# python2 is not packaged with spack so these features are disabled unless a | ||
# user has an external python2 package configured. | ||
# this will be fixed in the next release of nmap | ||
depends_on("python@:2", when="+ndiff") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi David! Thanks so much for this contribution!!!
(1) What version of nmap is expected to fix this? It would be preferable to add a version constraint with an upper bound to the when
clause in order to shut off this dependency for later nmap versions automatically. Please let me know if the documentation on version ranges is useful as I am working on it in a separate PR.
(2) What link did you use to determine this information (so we can see if the expected release date changes)? I use nmap infrequently but might file as maintainer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny seeing you here haha!
(1) The next release of nmap will add py3 support to their internal tools. I can add a version constraint for this dep easily here.
(2) I determined this from digging around in their repo, and this changelog: https://nmap.org/changelog.html
(2.1) Their configure script explicitly checks for python2 up until the unreleased version
To reply to both comments (@tldahlgren) do you all prefer disabling these features completely until the next nmap release, or should we keep the py2 dep but gate it by version? Either way changes will need to be made at update time so I sort of prefer just removing these feature flags until the next nmap release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question. I'm torn as I don't like completely removing an option that can soon be restored. But purists won't want the code until it/they are available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed the "just remove the features" version, but I'm seeing a different build issue now on my linux machine, and on my m1 mac.
On linux there's an openssl issue, on m1 there seems to be a build system race. I'll keep poking at this
Perhaps it's better to remove those options and document that the features cannot be used if/until Python3 is supported by them? |
args.append("--disable-ndiff") | ||
args.append("--disable-zenmap") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach. Shows these are options and explains they can be re-enabled once the codes support a reasonable (😉 ) Python.
okay, I believe I have fixed the mac build (with a bit of a hack). Is there a good way to check that all listed versions of this package build with any automation? I've just been checking the latest two |
A simple
seems to do the trick. A number of the versions do not build. A few short logs here: https://gist.github.com/dpzmick/040ddafe401bb43d039efe474f462d5a and a bunch of these versions depend on a deprecated openssl. I think these should all be cleaned up, but likely on a separate PR. This PR should be good to go! Thanks for the feedback, let me know if there's anything else that needs adjusting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can successfully install the new version.
I also made a few suggestions on comment wording. @cosmicexplorer Did you want to re-review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See suggested changes to wording of comments.
Interesting. I hoped that the comment would replace the required changes status for my review. |
Co-authored-by: Tamara Dahlgren <35777542+tldahlgren@users.noreply.github.com>
Co-authored-by: Tamara Dahlgren <35777542+tldahlgren@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the changes!
@cosmicexplorer Did you want to comment on this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tldahlgren reviewed:
I like this approach. Shows these are options and explains they can be re-enabled once the codes support a reasonable (😉 ) Python.
Indeed. I tested the build on x86 and aarch64-linux, and since python2 is removed it's a great fix too!
Co-authored-by: Tamara Dahlgren <35777542+tldahlgren@users.noreply.github.com>
Co-authored-by: Tamara Dahlgren <35777542+tldahlgren@users.noreply.github.com>
Co-authored-by: Tamara Dahlgren <35777542+tldahlgren@users.noreply.github.com>
Co-authored-by: Tamara Dahlgren <35777542+tldahlgren@users.noreply.github.com>
Co-authored-by: Tamara Dahlgren <35777542+tldahlgren@users.noreply.github.com>
I'm not sure how to do this exactly. It's failing CI due to a missing python2
this is also still getting issues on m1 darwin for me (but not my linux machines)