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

libzmq: modernize and deprecate @:4.3.2 #40399

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

Conversation

dennisklein
Copy link
Contributor

@dennisklein dennisklein commented Oct 10, 2023

I have grepped the builtin repo for libzmq dependents and did not find any conflicts with the proposed deprecation.

Note, the libzmq v4.3.3 release is already three years old.

@dennisklein dennisklein mentioned this pull request Oct 10, 2023
@dennisklein dennisklein changed the title libzmq: modernize and deprecrate @:4.3.2 libzmq: modernize and deprecate @:4.3.2 Oct 10, 2023
# @:4.0 legacy release 2016 (http://wiki.zeromq.org/intro:get-the-software),
# http://download.zeromq.org/zeromq-4.0.7.tar.gz -> 503 Service Unavailable

def deprecated(*values, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

can you just use version(..., deprecated=True) please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would make the line hit the length limit and reformat each version into a 5-line beast hurting readability a lot, unfortunately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you feel about adding it as an official DSL helper? e.g.

--- a/lib/spack/spack/directives.py
+++ b/lib/spack/spack/directives.py
@@ -25,7 +25,7 @@ class OpenMpi(Package):
   * ``provides``
   * ``resource``
   * ``variant``
-  * ``version``
+  * ``version`` (``deprecated``)
   * ``requires``

 """
@@ -70,6 +70,7 @@ class OpenMpi(Package):
     "resource",
     "build_system",
     "requires",
+    "deprecated",
 ]

 #: These are variant names used by Spack internally; packages can't use them
@@ -77,7 +78,7 @@ class OpenMpi(Package):

 #: Names of possible directives. This list is mostly populated using the @directive decorator.
 #: Some directives leverage others and in that case are not automatically added.
-directive_names = ["build_system"]
+directive_names = ["build_system", "deprecated"]

 _patch_order_index = 0

@@ -490,6 +491,15 @@ def _depends_on(pkg, spec, when=None, type=dt.DEFAULT_TYPES, patches=None):
         execute_patch(dependency)


+def deprecated(*values, **kwargs):
+    """Helper to declare a deprecated version
+
+    which fits into a single line for the standard case
+    """
+    kwargs["deprecated"] = True
+    return version(*values, **kwargs)
+
+
 @directive("conflicts")
 def conflicts(conflict_spec, when=None, msg=None):
     """Allows a package to define a conflict.
--- a/var/spack/repos/builtin/packages/libzmq/package.py
+++ b/var/spack/repos/builtin/packages/libzmq/package.py
@@ -46,10 +46,6 @@ def url_for_version(self, ver):
     #   @:4.0   legacy release 2016 (http://wiki.zeromq.org/intro:get-the-software),
     #           http://download.zeromq.org/zeromq-4.0.7.tar.gz -> 503 Service Unavailable

-    def deprecated(*values, **kwargs):
-        kwargs["deprecated"] = True
-        return version(*values, **kwargs)
-
     deprecated("4.3.2", sha256="ebd7b5c830d6428956b67a0454a7f8cbed1de74b3b01e5c33c5378e22740f763")
     deprecated("4.3.1", sha256="bcbabe1e2c7d0eec4ed612e10b94b112dd5f06fcefa994a0c79a45d835cd21eb")
     deprecated("4.3.0", sha256="8e9c3af6dc5a8540b356697081303be392ade3f014615028b3c896d0148397fd")

Copy link
Member

Choose a reason for hiding this comment

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

Let's just keep it declarative instead of inventing package specific ad-hoc syntax. #39964 should improve this in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#39964 should improve this in the future.

Alright, removed the helper. (I mistakenly pushed the wrong branch in between, sry for the noise)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied now the new context manager


class AutotoolsBuilder(autotools.AutotoolsBuilder):
Copy link
Member

@haampie haampie Oct 10, 2023

Choose a reason for hiding this comment

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

what's the point if there's just one build system? can you just leave it if it's irrelevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are other packages with only a single build system using this new API (mesa, lua-luafilesystem, glvis) from which I inferred it is the new API to be used?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyways, removed it.

maintainers("dennisklein")

conflicts("%gcc@8:", when="@:4.2.2")
Copy link
Member

Choose a reason for hiding this comment

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

versions usually go first, can you leave that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved them down again just above the depends_ons

Copy link
Member

@haampie haampie left a comment

Choose a reason for hiding this comment

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

See above

@dennisklein dennisklein force-pushed the libzmq-modernization-and-deprecation-campaign branch 2 times, most recently from 87e63f8 to 32c46d6 Compare October 10, 2023 09:37
@dennisklein dennisklein force-pushed the libzmq-modernization-and-deprecation-campaign branch from 32c46d6 to 6c7b2e1 Compare October 10, 2023 10:36
@dennisklein dennisklein reopened this Oct 10, 2023
@dennisklein dennisklein force-pushed the libzmq-modernization-and-deprecation-campaign branch 2 times, most recently from 9f806bc to eb899a4 Compare October 12, 2023 20:10
@dennisklein dennisklein marked this pull request as draft October 12, 2023 20:19
@dennisklein dennisklein force-pushed the libzmq-modernization-and-deprecation-campaign branch from eb899a4 to dce9447 Compare October 16, 2023 14:04
@spackbot-app spackbot-app bot added the stand-alone-tests Stand-alone (or smoke) tests for installed packages label Oct 17, 2023
@dennisklein dennisklein marked this pull request as ready for review October 17, 2023 01:02
Copy link
Member

@ChristianTackeGSI ChristianTackeGSI left a comment

Choose a reason for hiding this comment

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

This looks good to me!

I have one question (in as an inline comment).

var/spack/repos/builtin/packages/libzmq/package.py Outdated Show resolved Hide resolved
@dennisklein dennisklein force-pushed the libzmq-modernization-and-deprecation-campaign branch from c4ea28d to 2961b7b Compare November 10, 2023 16:40
When building from dist tarballs, docs come pre-compiled, so no extra
dependency is needed. On builds from a git working tree `libzmq` actually
used to depend on `asciidoc` and `xmlto` which was recently modernized
to `ruby-asciidoctor`, see zeromq/libzmq#4607
@dennisklein dennisklein force-pushed the libzmq-modernization-and-deprecation-campaign branch from 2961b7b to 7bfa65a Compare November 10, 2023 16:43
@dennisklein
Copy link
Contributor Author

rebased because the recipe now depends on #39964 which was only merged few days ago.

@dennisklein
Copy link
Contributor Author

hmm, https://gitlab.spack.io/spack/spack/-/jobs/9044690 failed with:

spack.installer.InstallError: Install failed for libzmq. No such file in prefix: lib/libzmq.so

a bit further up in the log one can see

libtool: install: (cd /home/gitlab-runner-4/builds/builds/1QhdDUZt9/0/spack/spack/opt/spack/[padded-to-256-chars]/morepadding/linux-sle_hpc15-x86_64_v4/gcc-10.3.0/libzmq-4.3.5-zd26tyvbvdh6ldwdgsfgmnyfvwz35xsp/lib64 && { ln -s -f libzmq.so.5.2.5 libzmq.so || { rm -f libzmq.so && ln -s libzmq.so.5.2.5 libzmq.so; }; })

which explains the failed check: the lib directory is called lib64 on this platform.

@dennisklein
Copy link
Contributor Author

dennisklein commented Nov 14, 2023

I see three solutions:

A. Do not hardcode the lib (and include) paths in the sanity check DSL:

    sanity_check_is_file = [
        join_path("include", "zmq.h"),
        join_path("lib", "libzmq.so"),
        join_path("lib", "pkgconfig", "libzmq.pc"),
    ]

However, not sure, if spack has (or wants to have) any knowledge about those paths?

B. Force the build system to always install to lib. Is this a good idea?

C. Not use those sanity checks. However, one of them already has proven useful when upstream modernized their documentation target. So, I'd rather keep those.

@dennisklein dennisklein force-pushed the libzmq-modernization-and-deprecation-campaign branch from 8df9143 to 97662d8 Compare November 14, 2023 12:21
@dennisklein dennisklein force-pushed the libzmq-modernization-and-deprecation-campaign branch from 97662d8 to c966f12 Compare November 14, 2023 12:38
Spacks test suite has shown that on some CRAY SLES system the install
tree directories may change, e.g. `lib` becomes `lib64` which then
breaks the sanity file checks.
@dennisklein dennisklein force-pushed the libzmq-modernization-and-deprecation-campaign branch from c966f12 to 074f74d Compare November 14, 2023 12:43
@dennisklein
Copy link
Contributor Author

ping?

@dennisklein
Copy link
Contributor Author

dennisklein commented May 8, 2024

What is the preferred way to move this forward (besides rebasing)? Shall I split it into smaller PRs? Are you opposing any proposed changes I should drop (in other words, are there parts of this PR which are good enough for merging)?

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.

None yet

3 participants