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

Integration test should work for Mac OS. #334

Merged

Conversation

MarcusSorealheis
Copy link
Collaborator

@MarcusSorealheis MarcusSorealheis commented Oct 21, 2023

Description

Minor issues related to MacOS for the integration tests.

Fixes # #316

Not technically an issue. It's blocking an in-progress PR.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please also list any relevant details for your test configuration

Checklist

  • Updated documentation if needed
  • Tests added/amended
  • bazel test //... passes locally
  • PR is contained in a single commit, using git amend see some docs

This change is Reviewable

Copy link
Collaborator

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @MarcusSorealheis)


run_integration_tests.sh line 19 at r1 (raw file):

if [[ $EUID -eq 0 ]]; then
  echo "This script shoudl not be run as root."

Misspelled.


run_integration_tests.sh line 49 at r1 (raw file):

done

echo "What operating system OSTYPE: $OSTYPE"

This ENV is not set on all OSs.


run_integration_tests.sh line 59 at r1 (raw file):

  TEST_PATTERNS=("*")
fi

Needless new line.


run_integration_tests.sh line 73 at r1 (raw file):

sudo docker-compose rm --stop -f

echo "What operating system OSTYPE: $OSTYPE"

nit: ditto


run_integration_tests.sh line 77 at r1 (raw file):

export TMPDIR=$HOME/.cache/turbo-cache/
sudo mkdir -p "$TMPDIR"

don't mkdir with sudo, it causes issues.


run_integration_tests.sh line 79 at r1 (raw file):

sudo mkdir -p "$TMPDIR"

if [[ "$OSTYPE" == "linux-gnu"* ]]; then

use ${OSTYPE:-linux-gnu}


run_integration_tests.sh line 82 at r1 (raw file):

  export CACHE_DIR=$(mktemp -d --tmpdir="$TMPDIR" --suffix="-turbo-cache-integration-test")
elif [[ "$OSTYPE" == "darwin"* ]]; then
  # Create a temporary directory using mktemp with a random template, then add a suffix

nit: end in a period.


run_integration_tests.sh line 83 at r1 (raw file):

elif [[ "$OSTYPE" == "darwin"* ]]; then
  # Create a temporary directory using mktemp with a random template, then add a suffix
  export CACHE_DIR=$(mktemp -d "${TMPDIR}turbo-cache-integration-test")

nit: add slash between ${TMPDIR} and the constant.


run_integration_tests.sh line 84 at r1 (raw file):

  # Create a temporary directory using mktemp with a random template, then add a suffix
  export CACHE_DIR=$(mktemp -d "${TMPDIR}turbo-cache-integration-test")
  echo "CACHE_DIR for macOS is: $CACHE_DIR"  # <-- Added print statement here

nit: remove this.


run_integration_tests.sh line 85 at r1 (raw file):

  export CACHE_DIR=$(mktemp -d "${TMPDIR}turbo-cache-integration-test")
  echo "CACHE_DIR for macOS is: $CACHE_DIR"  # <-- Added print statement here
  # exit 1

nit: remove this.


run_integration_tests.sh line 88 at r1 (raw file):

else
  # Unknown OS
  echo "Unknown operating system. Exiting."

nit: Lets instead invert this and assume linux if it's not mac.


run_integration_tests.sh line 106 at r1 (raw file):

    # Cleanup.
    echo "Cleaning up cache directories TURBOC_CACHE_DIR: $TURBO_CACHE_DIR"
    

nit: Needless spaces.

@MarcusSorealheis MarcusSorealheis force-pushed the fix-integration-for-macOS branch 2 times, most recently from c6980b0 to 6b37ecf Compare October 21, 2023 16:31
Copy link
Collaborator Author

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 10 unresolved discussions (waiting on @allada)


run_integration_tests.sh line 73 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: ditto

Removed.


run_integration_tests.sh line 77 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

don't mkdir with sudo, it causes issues.

Done


run_integration_tests.sh line 79 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

use ${OSTYPE:-linux-gnu}

Done.


run_integration_tests.sh line 83 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: add slash between ${TMPDIR} and the constant.

$TEMP_DIR has a trailing slash.


run_integration_tests.sh line 88 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: Lets instead invert this and assume linux if it's not mac.

what about Windows? Some customers use Windows.

Copy link
Collaborator Author

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 10 unresolved discussions (waiting on @allada)


run_integration_tests.sh line 79 at r1 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

Done.

looks like it should be linux-gnu as I had before

Copy link
Collaborator Author

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 10 unresolved discussions (waiting on @allada)


run_integration_tests.sh line 79 at r1 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

looks like it should be linux-gnu as I had before

https://github.com/TraceMachina/turbo-cache/actions/runs/6598502029/job/17926538604

Copy link
Collaborator Author

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 10 unresolved discussions (waiting on @allada)


run_integration_tests.sh line 79 at r1 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

https://github.com/TraceMachina/turbo-cache/actions/runs/6598502029/job/17926538604

Should I write a regex for either or?

@MarcusSorealheis MarcusSorealheis force-pushed the fix-integration-for-macOS branch 6 times, most recently from 5418163 to 105f34a Compare October 21, 2023 18:08
@MarcusSorealheis
Copy link
Collaborator Author

@allada ball in your court but curious about your thoughts on $OS_TYPE.

Copy link
Collaborator Author

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

My overall opinion is that this works but I do not want to put too much effort into it given it will go away in the near term.

Reviewable status: 1 of 2 files reviewed, 9 unresolved discussions (waiting on @allada)


run_integration_tests.sh line 19 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Misspelled.

Done.


run_integration_tests.sh line 59 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Needless new line.

Done.

Copy link
Collaborator

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @MarcusSorealheis)


run_integration_tests.sh line 79 at r1 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

Should I write a regex for either or?

The command I gave you just defaults OSTYPE to linux-gnu if it is not set.

It should look like this:

if [[ "${OSTYPE:-linux-gnu}" == "linux-gnu"* ]]; then

run_integration_tests.sh line 88 at r1 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

what about Windows? Some customers use Windows.

This test doesn't work on windows anyway.

Copy link
Collaborator Author

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @allada)


run_integration_tests.sh line 49 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

This ENV is not set on all OSs.

ahh. I see what you meant. This script would fail on those platforms.


run_integration_tests.sh line 79 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

The command I gave you just defaults OSTYPE to linux-gnu if it is not set.

It should look like this:

if [[ "${OSTYPE:-linux-gnu}" == "linux-gnu"* ]]; then

that wasn't clear. I get default parameter, though. I thought you only specify the string.

Copy link
Collaborator Author

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @allada)


run_integration_tests.sh line 79 at r1 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

that wasn't clear. I get default parameter, though. I thought you only specify the string.

I think this is fine now

Copy link
Collaborator

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r7.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @MarcusSorealheis)


run_integration_tests.sh line 79 at r7 (raw file):

  export CACHE_DIR=$(mktemp -d "${TMPDIR}turbo-cache-integration-test")
else
  # Unknown OS

nit: needless comment.


run_integration_tests.sh line 100 at r7 (raw file):

    echo "Checking for existince of the TURBO_CACHE_DIR"
    if [ -d "$TURBO_CACHE_DIR" ]; then
      sudo find "$TURBO_CACHE_DIR" -delete # add for linux

nit: Needless comment.

Copy link
Collaborator

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @MarcusSorealheis)


run_integration_tests.sh line 79 at r8 (raw file):

else
  echo "Unable to detect operating system. Assuming the Linux/WSL syntax will work."
  export CACHE_DIR=$(mktemp -d --tmpdir="$TMPDIR" --suffix="-turbo-cache-integration-test")

Since we are doing the same thing in:
if [[ "${OSTYPE:-linux-gnu}" == "linux-gnu"* ]]; then

Can we instead just check darwin and then assume linux otherwise and get rid of this else clause?

Copy link
Collaborator Author

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @allada)


run_integration_tests.sh line 79 at r8 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Since we are doing the same thing in:
if [[ "${OSTYPE:-linux-gnu}" == "linux-gnu"* ]]; then

Can we instead just check darwin and then assume linux otherwise and get rid of this else clause?

sure

Copy link
Collaborator

@allada allada left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @MarcusSorealheis)

Copy link
Collaborator

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r10.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @MarcusSorealheis)

@MarcusSorealheis MarcusSorealheis merged commit 1339e9d into TraceMachina:main Oct 21, 2023
12 of 15 checks passed
@MarcusSorealheis MarcusSorealheis deleted the fix-integration-for-macOS branch October 21, 2023 22:04
blakehatch pushed a commit to blakehatch/nativelink that referenced this pull request Nov 14, 2023
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.

None yet

2 participants