Environment/depfile: fix bug with Git hash versions (attempt #2)#37560
Environment/depfile: fix bug with Git hash versions (attempt #2)#37560haampie merged 9 commits intospack:developfrom
Conversation
spackbot-app
bot
added
commands
core
scheibelp
changed the title
scheibelp
changed the title
|
I noticed that this escaping problem is not limited to diff --git a/var/spack/repos/builtin/packages/zlib/package.py b/var/spack/repos/builtin/packages/zlib/package.py
index 16341ecbaf..0e3e580aed 100644
--- a/var/spack/repos/builtin/packages/zlib/package.py
+++ b/var/spack/repos/builtin/packages/zlib/package.py
@@ -22,6 +22,7 @@ class Zlib(MakefilePackage, Package):
homepage = "https://zlib.net"
# URL must remain http:// so Spack can bootstrap curl
url = "http://zlib.net/fossils/zlib-1.2.11.tar.gz"
+ git = "https://github.com/madler/zlib.git"
version("1.2.13", sha256="b3a24de97a8fdbc835b9833169501030b8977031bcb54b3b3ac13740f846ab30")
version(and install It generates a broken Makefile due to So... I wonder if a safe Spec.format function would be good to have in general? We'll need it in projections. |
The concept of "safe" can change depending on the context, so I'm not sure if there's a one-size-fits-all definition of safe (perhaps there is some "very safe" format which would satisfy all needs). If you are proposing defining a I also think most of the work is in knowing where to call it vs. whether to have it. In the case you brought up, it sounds like |
| unsafe_result = self.spec.format(format_str) | ||
| if not MakefileSpec._pattern: | ||
| MakefileSpec._pattern = re.compile(r"[^A-Za-z0-9_.-]") | ||
| return MakefileSpec._pattern.sub("_", unsafe_result) |
There was a problem hiding this comment.
@haampie if I understand from our conversation this morning, you are saying this re.sub call could be moved into Spec.format itself (i.e. it would take another parameter like filter_regex, in which case that provides a way to resolve zlib's =-in-prefix issue). That is true but that consolidation doesn't seem to provide much benefit (i.e. moving that into Spec doesn't eliminate work on the caller's part other than that they wouldn't have to import re anymore).
| return True | ||
|
|
||
|
|
||
| class MakefileSpec(object): |
There was a problem hiding this comment.
| class MakefileSpec(object): | |
| class MakefileSpec: |
lib/spack/spack/test/cmd/env.py
Outdated
| else: | ||
| return orig_format(obj, format_str) | ||
|
|
||
| monkeypatch.setattr(spack.spec.Spec, "format", _contrived_spec_format) |
There was a problem hiding this comment.
I'm sorry, but I still don't find this test acceptable. Don't mock internals, they may be relied on elsewhere, and only make refactoring hard. I'll just push an integration/regression test for git version, and a unit test for your formatter.
… enter Makefile objects
haampie
force-pushed
the
bugfix/depfile-hash-eq-ver2
branch
from
50d48f8 to
140d8d1
Compare
haampie
force-pushed
the
bugfix/depfile-hash-eq-ver2
branch
from
140d8d1 to
b645f22
Compare
…ck#37560) Co-authored-by: Harmen Stoppels <[email protected]>
…spack#37560) Co-authored-by: Harmen Stoppels <[email protected]>
2 participants
Replaces #36642
If a spec contains a hash=number version, this was resulting in malformed Makefiles generated by spack env depfile (the target cannot include an un-escaped '=' character). This
Specs in theMakefileModel(and associated objects) with references to a limited interface to avoid similar issues in the futureTODO