Skip to content

Move OS specific code into its own folder#34779

Closed
alalazo wants to merge 1 commit intospack:developfrom
alalazo:maintainers/os_specific_code
Closed

Move OS specific code into its own folder#34779
alalazo wants to merge 1 commit intospack:developfrom
alalazo:maintainers/os_specific_code

Conversation

@alalazo
Copy link
Copy Markdown
Member

@alalazo alalazo commented Jan 3, 2023

Move all the llnl.* code that is either Windows or Unix specific into separate folders. The API for both platforms is the same and the correct code is imported based on "sys.platform".

Modifications:

  • Move platform specific code into separate folders
  • Import in llnl.util.filesystem and llnl.util.symlink the correct implementation based on the current sys.platform

@spackbot-app spackbot-app bot added compilers core PR affects Spack core functionality fetching tests General test capability(ies) utilities labels Jan 3, 2023
@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Jan 3, 2023

This is an attempt to avoid having is_windows appearing into multiple places within a package. If the approach seems better than the current state it can be extended to other parts in spack.* code. Any comment on the layout used in this PR is welcome.

alalazo
alalazo commented Jan 3, 2023
View reviewed changes
lib/spack/llnl/util/filesystem.py
def getuid():
if is_windows:
import ctypes
if sys.platform == "win32":
Copy link
Copy Markdown
Member Author

@alalazo alalazo Jan 3, 2023

Choose a reason for hiding this comment

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

The assumption here is that we'll import different implementations of the same functions in both conditional branches.

@alalazo
Move OS specific code into their own folder
6d45358 
Move all the code that is either Windows or Unix specific
into separate folders. The API for both platforms is the same
and the correct code is imported based on "sys.platform".
@alalazo alalazo force-pushed the maintainers/os_specific_code branch from acacb98 to 6d45358 Compare January 3, 2023 16:13
@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Jan 4, 2023

@johnwparent @BetsyMcPhail @tgamblin @scheibelp fyi let me know if this approach is worth polishing / applying to other parts of the code.

@scheibelp scheibelp self-assigned this Jan 11, 2023
tldahlgren
tldahlgren reviewed Jan 11, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@tldahlgren tldahlgren left a comment

Choose a reason for hiding this comment

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

Real quick pass I noticed there are a couple of copyright end years off.

lib/spack/llnl/util/_windows/__init__.py
@@ -0,0 +1,36 @@
# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
Copy link
Copy Markdown
Contributor

@tldahlgren tldahlgren Jan 11, 2023

Choose a reason for hiding this comment

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

Suggested change
# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other

lib/spack/llnl/util/_windows/symlink.py
@@ -0,0 +1,104 @@
# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
Copy link
Copy Markdown
Contributor

@tldahlgren tldahlgren Jan 11, 2023

Choose a reason for hiding this comment

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

Suggested change
# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other

Copy link
Copy Markdown
Member Author

@alalazo alalazo Jan 11, 2023

Choose a reason for hiding this comment

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

Great catch, I wonder where I copied those from and more importantly why CI was passing 🤔

@tldahlgren tldahlgren requested a review from scheibelp January 11, 2023 17:28
@alalazo alalazo mentioned this pull request Feb 23, 2023
Merged
@scheibelp
Copy link
Copy Markdown
Member

scheibelp commented Feb 23, 2023

My general preference is that if a method can abstract Windows-specific details, then having is_windows checks inside of it is good and can avoid code duplication. I should add specifics here but wanted to voice that general concern.

@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Nov 26, 2024

Since this was just a proof of concept, and is now outdated, close it.

@alalazo alalazo closed this Nov 26, 2024
@alalazo alalazo deleted the maintainers/os_specific_code branch November 26, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tldahlgren tldahlgren tldahlgren left review comments

@scheibelp scheibelp Awaiting requested review from scheibelp

Assignees

@scheibelp scheibelp

Labels

compilers core PR affects Spack core functionality fetching tests General test capability(ies) utilities

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alalazo @scheibelp @tldahlgren