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

Deprecate/Remove Cygwin support #134

Closed
bryanwweber opened this issue Feb 1, 2022 · 4 comments · Fixed by Cantera/cantera#1367
Closed

Deprecate/Remove Cygwin support #134

bryanwweber opened this issue Feb 1, 2022 · 4 comments · Fixed by Cantera/cantera#1367
Labels
feature-request New feature request

Comments

@bryanwweber
Copy link
Member

bryanwweber commented Feb 1, 2022

Abstract

Cygwin support is untested and Cygwin has uncertain usage for Cantera. With the advent of (potentially) better replacements such as the Windows Subsystem for Linux v2 (WSL2), Cygwin support may not be necessary any more.

Motivation

Cygwin support imposes a mild burden on maintainers, with several untested code paths in various SCons* files. Retaining these support features without testing that they work is perhaps misleading to end users, if they don't work. Setting up and maintaining a test setup for Cygwin would likely not be worth the anticipated usage.

Possible Solutions

  1. Remove the following lines from the codebase, found by grep -irn --exclude-dir=ext "cygwin" .:

    ./SConstruct:202:            "Cygwin": "-std=gnu++11", # See http://stackoverflow.com/questions/18784112
    ./SConstruct:740:if "cygwin" in env["OS"].lower():
    ./SConstruct:741:    env["OS"] = "Cygwin" # remove Windows version suffix
    ./SConstruct:781:    if env["OS"] == "Cygwin":
    ./SConstruct:782:        config.select("Cygwin")
    ./test_problems/SConscript:28:if env['OS'] == 'Windows' or env['OS'] == 'Cygwin':
    ./site_scons/buildutils.py:1196:# Monkey patch for SCons Cygwin bug
    ./site_scons/buildutils.py:1198:if "cygwin" in platform.system().lower():
    ./interfaces/cython/SConscript:96:elif localenv['OS'] == 'Cygwin':
    
  2. Do part 1, and also raise a ValueError at the top of SConstruct if the OS is found to be Cygwin encouraging users to post on this issue that they require ongoing support. Remove the ValueError after the release of Cantera 2.6.

  3. Leave the code as-is.

  4. Add a testing environment for Cygwin

My personal preference is for number 2.

@bryanwweber bryanwweber added the feature-request New feature request label Feb 1, 2022
@ischoegl
Copy link
Member

ischoegl commented Feb 1, 2022

I'm supporting (2) also, although a laconic statement similar to this one may suffice: https://github.com/Cantera/cantera/blob/9cee0525d26b6030cc30c469c9c75332d836eb8e/SConstruct#L86-L88 (e.g. replace 'unrecognized' by 'not supported')

... if necessary (which I do not think it is), I would strongly advocate moving to WSL instead, where a GH Action exists for setup. At the same time, there also is a GH Action for Cygwin, but my suspicion is that WSL may already work out of the box.

@bryanwweber
Copy link
Member Author

@ischoegl Do you think we need to test WSL support? It should be a standard Linux distribution, so equivalent to our other Linux test cases, right?

@ischoegl
Copy link
Member

ischoegl commented Feb 1, 2022

Do you think we need to test WSL support? It should be a standard Linux distribution, so equivalent to our other Linux test cases, right?

@bryanwweber ... agreed (I guess this means a no). I would suspect that it 'just works', but never had a reason to try. I believe cygwin was more relevant when compiler options were less abundant on Windows, but this is probably a thing of the past.

@bryanwweber
Copy link
Member Author

I believe cygwin was more relevant when compiler options were less abundant on Windows, but this is probably a thing of the past.

Yes, I used Cygwin once upon a time... Probably back in the WinXP days before I "upgraded" to Vista and then truly upgraded to Win7 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request New feature request
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants