Skip to content

Conversation

@jhunkeler
Copy link
Contributor

Resolves SCSB-201

This PR makes it possible to compile stcal using MSVC/MSBuild on Windows.

  • MSVC does not support Variable Length Arrays (ref)
  • POSIX resource management functions do not exist on Windows (sys/resource.h, etc.).
    • If need be, we can implement the same functionality using the Windows API

Tasks

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • run regression tests with this branch installed ("git+https://github.com/<fork>/stcal@<branch>")
news fragment change types...
  • changes/<PR#>.apichange.rst: change to public API
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change

@jhunkeler jhunkeler requested a review from a team as a code owner March 10, 2025 17:19
@codecov
Copy link

codecov bot commented Mar 10, 2025

Codecov Report

Attention: Patch coverage is 33.33333% with 12 lines in your changes missing coverage. Please review.

Project coverage is 89.35%. Comparing base (bb57168) to head (1c9127f).
Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
setup.py 0.00% 10 Missing ⚠️
src/stcal/outlier_detection/median.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #351      +/-   ##
==========================================
- Coverage   89.41%   89.35%   -0.06%     
==========================================
  Files          64       64              
  Lines       10143    10172      +29     
==========================================
+ Hits         9069     9089      +20     
- Misses       1074     1083       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pllim
Copy link
Contributor

pllim commented Mar 14, 2025

Should add a Windows build here to prove this PR works. 😉

Copy link
Member

@mcara mcara left a comment

Choose a reason for hiding this comment

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

Amazing!

@jhunkeler
Copy link
Contributor Author

Of course the moment I decide to add Windows into the mix:
python/cpython#135151

@pllim
Copy link
Contributor

pllim commented Jun 13, 2025

You finally get to the fun stuff:

assert str(arr._filename).split("/")[-1] in os.listdir(tempdir)

and

>       return io.open(self, mode, buffering, encoding, errors, newline)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
E       PermissionError: [Errno 13] Permission denied: 'C:\\...\\test_disk_appendable_array_bad0\\tmptest'

* On Windows opening a directory as a file results in a "PermissionError" exception
* Here we trap the exception, and if we're on Windows, check whether the file name is a directory. If so raise an "IsADirectoryError" exception
* "fname" gives us the absolute path to the file, so there's no need to
  list the destination directory to see if the file is in there.
  fname.exists will suffice.
@jhunkeler
Copy link
Contributor Author

Tada.
https://github.com/spacetelescope/stcal/actions/runs/15644177990/job/44078328011?pr=351

@pllim
Copy link
Contributor

pllim commented Jun 13, 2025

Maybe this warrants a change log so Windows user can read it and be happy.

Removes noqa comment

Co-authored-by: P. L. Lim <[email protected]>
@pllim
Copy link
Contributor

pllim commented Jun 13, 2025

FWIW green job is good enough for merge. We can always follow-up later if there is any Windows intricacies to fix later (I think that is going to get smoke out if downstream also start doing regression tests in Windows 👹). But since I am not maintainers, I'll leave approval and merge to someone else. Thanks!

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.

3 participants