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

mason search broken #24495

Closed
lucaferranti opened this issue Feb 28, 2024 · 7 comments
Closed

mason search broken #24495

lucaferranti opened this issue Feb 28, 2024 · 7 comments

Comments

@lucaferranti
Copy link
Contributor

both in 1.33 and on main

❯ mason search
Updating mason-registry
MasonUtils.chpl:255: error: zippered iterations have non-equal lengths
@lucaferranti
Copy link
Contributor Author

lucaferranti commented Feb 28, 2024

this happens in MasonUtils.chpl:255

 proc init(str:string) {
    const s : [1..3] string = str.split(".");
    assert(s.size == 3);

    major = s[1]:int;
    minor = s[2]:int;
    bug   = s[3]:int;
  }

the error is returned because str does not contain a . to split into three strings and hence cannot convert to [1..3] str. To debug this, I added a writeln(str); before the const s definition and rebuilt mason, the result was

❯ bin/linux64-x86_64/mason search
0.19.0
Updating mason-registry
0.1.0
0.1.0
0.1.0
0.1.0
0.1.0
0.1.0
Mason
MasonUtils.chpl:256: error: zippered iterations have non-equal lengths

the incriminating string is the last one Mason.

Note also that the next assert is not needed, because the previous line will work iff str can be split into an array of 3 elements. A more robust way would be to do

 proc init(str:string) {
    const s = str.split(".");
    assert(isArray(s) && s.size == 3, "Invalid version " + str);

    major = s[0]:int;
    minor = s[1]:int;
    bug   = s[2]:int;
  }

this would give a more informative error message if one tries to construct an invalid version, but does not solve the problem, since we want mason search to work, not to fail informatively :)

I suspect the last Mason string which casues the error is due to reading too much data from the registry (but haven't verified), probably want a higher level sanity check

edit: alternatively, one could use semver's recommended regex to check validity and extract elements

@lucaferranti
Copy link
Contributor Author

lucaferranti commented Feb 28, 2024

implementing my suggestion does indeed confirm what I was thinking

❯ bin/linux64-x86_64/mason search
Updating mason-registry
MasonUtils.chpl:256: error: assert failed - Invalid version Mason

but I am still unsure where the Mason comes from. Is there a way to get mason to print the whole call stacktrace when it hits an error?

@bradcray
Copy link
Member

@lucaferranti : Answering your last question, would CHPL_UNWIND help here? https://chapel-lang.org/docs/usingchapel/chplenv.html#chpl-unwind

@lucaferranti
Copy link
Contributor Author

lucaferranti commented Feb 28, 2024

figured out the issue!

The incriminating package in the mason registry is Codecs because for that one the file is called Mason.toml instead of x.y.z.toml. This causes issues beause findLatest in MasonUtils.chpl currently does

    const end = manifest.size - suffix.size;
    const ver = new VersionInfo(manifest[0..<end]);
    if ver > ret then ret = ver;

where manifest is the filename, so as you can see it assumes that the toml file is named after the version, which breaks for Codecs.

A quick fix is to patch the toml file name for codecs. However in general I think mason-registry have a bash (or chapel :) ) script checkign that the toml file is named properly (from what I can say, this is not the case atm) as well as other sanity checks.

@arezaii
Copy link
Contributor

arezaii commented Mar 5, 2024

I have merged chapel-lang/mason-registry#70, which should resolve the broken search feature. @lucaferranti, if you'd like to file a PR to update the init proc you identified here I would be happy to merge it on your behalf.

@bradcray
Copy link
Member

@arezaii / @lucaferranti : Checking in to see what it would take to close this issue. Is the init improvement required, or just a nice failsafe? (sounds like the latter to me). Do either of you want to take that up, or should we just close this? Thanks.

@lucaferranti
Copy link
Contributor Author

Yes, the init change I suggested above would make the error message more informative but not fix the issue if an incorrectly named file occurs. I think this can be closed for now, I can open a PR with the slight improvement in the init handling later this week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants