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

update rename ~/.benchmark to ~/.osb and add symlink #721

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

OVI3D0
Copy link
Member

@OVI3D0 OVI3D0 commented Jan 2, 2025

Description

Updates OSB's config directory to ~/.osb instead of ~/.benchmark. Also adds a symlink if the old path exists but the new one does not. 'benchmark' is an overloaded term, while using ~/.osb reduces ambiguity and is more consistent with the project's name.

Issues Resolved

[List any issues this PR will resolve]

Testing

  • [] New functionality includes testing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Michael Oviedo <mikeovi@amazon.com>
@OVI3D0 OVI3D0 marked this pull request as ready for review January 2, 2025 22:01
@gkamat gkamat self-requested a review January 2, 2025 23:47
new_path = os.path.join(default_home, ".osb")
if os.path.exists(old_path) and not os.path.exists(new_path):
os.symlink(old_path, new_path)
return os.path.join(os.getenv("BENCHMARK_HOME", default_home), ".osb")
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the .benchmark directory does not exist, you will need to create it, so both are present in all cases.

Signed-off-by: Michael Oviedo <mikeovi@amazon.com>
@OVI3D0
Copy link
Member Author

OVI3D0 commented Jan 3, 2025

@gkamat could there be a permissions issue when creating the symlink in the integ tests?
it/distribution_test.py::test_tar_distributions[os-it] Warning: Failed to create symlink from /home/runner/.osb to /home/runner/.benchmark

@IanHoang
Copy link
Collaborator

IanHoang commented Jan 7, 2025

We'll need to change the direction of this PR so that it merges into a separate branch as this is somewhat a breaking change (despite the symlink, documentation has not been updated)?


# Create .benchmark directory if it doesn't exist
if not os.path.exists(old_path):
os.makedirs(old_path, exist_ok=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can reuse the ensure_dir() from utils.

def ensure_dir(directory, mode=0o777):
    """
    Ensure that the provided directory and all of its parent directories exist.
    This function is safe to execute on existing directories (no op).

    :param directory: The directory to create (if it does not exist).
    :param mode: The permission flags to use (if it does not exist).
    """
    if directory:
        os.makedirs(directory, mode, exist_ok=True)

try:
os.symlink(old_path, new_path, target_is_directory=True)
except OSError:
print(f"Warning: Failed to create symlink from {new_path} to {old_path}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we also output the exact error?

@IanHoang
Copy link
Collaborator

IanHoang commented Jan 7, 2025

@gkamat could there be a permissions issue when creating the symlink in the integ tests? it/distribution_test.py::test_tar_distributions[os-it] Warning: Failed to create symlink from /home/runner/.osb to /home/runner/.benchmark

@OVI3D0 can we modify the exception clause to see what the exact error is from OSError?

…r message

Signed-off-by: Michael Oviedo <mikeovi@amazon.com>
@IanHoang
Copy link
Collaborator

IanHoang commented Jan 7, 2025

@OVI3D0 FileExistsError is showing up when os.symlink() is run. Can you add os.path.islink() to check if there is an existing symlink?

@OVI3D0
Copy link
Member Author

OVI3D0 commented Jan 7, 2025

@OVI3D0 FileExistsError is showing up when os.symlink() is run. Can you add os.path.islink() to check if there is an existing symlink?

I added a function that should hopefully stop these errors, called ensure_symlink in the utils file

@OVI3D0 OVI3D0 force-pushed the osb-symlink branch 9 times, most recently from be4ff74 to 44bf1d8 Compare January 7, 2025 22:49
Signed-off-by: Michael Oviedo <mikeovi@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants