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

Fix ansys installation finder #1966

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Fix ansys installation finder #1966

merged 1 commit into from
Dec 16, 2024

Conversation

FedericoNegri
Copy link
Contributor

@FedericoNegri FedericoNegri commented Dec 12, 2024

The changes in this PR broke the Ansys finder, at least on Linux. See code comments for a description of the changes.

Tested on a Linux machine with Ansys installed at /ansys_inc/v251 using a minimal script replicating the logic in the function find_ansys.

Original script:

import os
from pathlib import Path

def is_float(string):
    try:
        float(string)
        return True
    except ValueError:
        return False

def find_ansys():

    base_path = None
    if os.name == "nt":
        base_path = Path(os.environ["PROGRAMFILES"]) / "ANSYS INC"
    elif os.name == "posix":
        for path in [Path("/usr/ansys_inc"), Path("/ansys_inc")]:
            if path.is_dir():
                base_path = path
    else:
        raise OSError(f"Unsupported OS {os.name}")

    if base_path is None:
        return base_path

    paths = base_path.glob("v*")

    if not list(paths):
        return None

    versions = {}
    for path in paths:
        ver_str = str(path)[-3:]
        if is_float(ver_str):
            versions[int(ver_str)] = str(path)

    return versions[max(versions.keys())]

print(find_ansys())

Output:

Traceback (most recent call last):
  File "find_ansys_bug.py", line 39, in <module>
    print(find_ansys())
  File "find_ansys_bug.py", line 37, in find_ansys
    return versions[max(versions.keys())]
ValueError: max() arg is an empty sequence

Diff of fixed script (same as this PR):

--- a/find_ansys_bug.py
+++ b/find_ansys.py
@@ -23,9 +23,9 @@ def find_ansys():
     if base_path is None:
         return base_path

-    paths = base_path.glob("v*")
+    paths = list(base_path.glob("v*"))

-    if not list(paths):
+    if not paths:
         return None

Output:

/ansys_inc/v251

Note: codecov complains about 0% coverage but I'd say that if tests weren't failing before the fix, I have little chance to come up with a good test.


if not list(paths):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

paths is a generator, it gets exhausted after first read

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.37%. Comparing base (af30b32) to head (ca0ea56).
Report is 33 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1966      +/-   ##
==========================================
- Coverage   88.42%   88.37%   -0.06%     
==========================================
  Files          89       89              
  Lines       10265    10251      -14     
==========================================
- Hits         9077     9059      -18     
- Misses       1188     1192       +4     

@FedericoNegri FedericoNegri marked this pull request as ready for review December 12, 2024 16:58
@moe-ad
Copy link
Contributor

moe-ad commented Dec 14, 2024

Thanks for catching this @FedericoNegri .

@FedericoNegri
Copy link
Contributor Author

@cbellot000 I can't merge because of codecov

@cbellot000
Copy link
Contributor

@cbellot000 I can't merge because of codecov

It's supposed to be optional, do you see:
image
If yes, you can go ahead

@FedericoNegri FedericoNegri enabled auto-merge (squash) December 16, 2024 08:52
@FedericoNegri
Copy link
Contributor Author

@cbellot000
image

@cbellot000
Copy link
Contributor

@cbellot000 image

if it's ready, I can complete for you. Is that good with you?

@FedericoNegri
Copy link
Contributor Author

@cbellot000 yes, please merge it, thanks

@cbellot000 cbellot000 disabled auto-merge December 16, 2024 10:45
@cbellot000 cbellot000 merged commit 6f7cde8 into master Dec 16, 2024
68 of 69 checks passed
@cbellot000 cbellot000 deleted the fix/find-ansys-linux branch December 16, 2024 10:45
@PProfizi PProfizi added the bug Something isn't working label Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants