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.

Sign up for GitHub

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

Jump to bottom

Add exec-mode to script files #1486

Merged
merged 3 commits into from Nov 10, 2023
Merged

Add exec-mode to script files #1486

merged 3 commits into from Nov 10, 2023

Conversation

gargnitingoogle
Copy link
Collaborator

@gargnitingoogle gargnitingoogle commented Oct 31, 2023 •
edited

Description

Add execute mode to all shell scripts

This change adds execute mode (chmod +x)
to add script files in the gcsfuse project. This is needed to avoid having to
chmod +x each time for running a shell
script especially a in jobs or in other scripts.
This chmod +x caused errors in running git checkout
in the calling script in the scenario like below.

git checkout master
chmod +x some_child_script.sh
# run some tests
git checkout dev_branch ### FAILS saying files are modified.
# run same tests

This change addresses internal bug#307698513 .

This has a follow-up cleanup in Remove chmod calls for running scripts . Currently these two changes can't be merged together because putting them together fails the presubmit tests.

Link to the issue in case of a bug fix.

NA

Testing details

  1. Manual - NA
  2. Unit tests - All run locally and passed.
  3. Integration tests - Ran through presubmits

@gargnitingoogle gargnitingoogle added execute-perf-test Execute performance test in PR execute-integration-tests Run only integration tests labels Oct 31, 2023
@gargnitingoogle gargnitingoogle changed the base branch from master to dec_2023_release October 31, 2023 06:50
@gargnitingoogle gargnitingoogle changed the title Add exec-mode to all script files Add exec-mode to script files Oct 31, 2023
ashmeenkaur
ashmeenkaur reviewed Oct 31, 2023
View reviewed changes
Copy link
Collaborator

@ashmeenkaur ashmeenkaur left a comment

Choose a reason for hiding this comment

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

KOKORO tests are failing. Is this expected?

@gargnitingoogle
Copy link
Collaborator Author

KOKORO tests are failing. Is this expected?

No, I'll look at the failure.

@gargnitingoogle gargnitingoogle mentioned this pull request Nov 1, 2023
Merged
@gargnitingoogle gargnitingoogle force-pushed the gargnitin-fix-script-git-checkout-error branch from a03fd18 to 7dd8705 Compare November 1, 2023 10:59
@gargnitingoogle gargnitingoogle changed the base branch from dec_2023_release to master November 1, 2023 10:59
@gargnitingoogle
Copy link
Collaborator Author

KOKORO tests are failing. Is this expected?

No, I'll look at the failure.

The failure was because I had also removed the chmod calls also within the same change, which caused the presubmits to fail. The presubmit jobs do the following:

git checkout master
chmod +x run_some_script.sh # Earlier moved in this PR itself.

./run_some_script.sh
# Fails because even though we have add exec-permission on run_some_script.sh in this PR/branch, it's not reflected on the master branch until the merge goes through, and master branch doesn't have run_some_script.sh as executable.

# verify output
git checkout dev-branch # FAILS 
./run_some_script.sh

I've addressed this failure by splitting out the chmod removals into a cleanup PR afterwards i.e. Remove chmod calls for running scripts.

@gargnitingoogle
Copy link
Collaborator Author

This PR is safe to be merged into the release branch along the same line as here .

@gargnitingoogle gargnitingoogle force-pushed the gargnitin-fix-script-git-checkout-error branch 2 times, most recently from 704056e to 65de916 Compare November 2, 2023 05:11
@gargnitingoogle gargnitingoogle removed the execute-integration-tests Run only integration tests label Nov 2, 2023
ashmeenkaur
ashmeenkaur previously approved these changes Nov 6, 2023
View reviewed changes
@raj-prince
Copy link
Collaborator

raj-prince commented Nov 7, 2023 •
edited

I can see there are 21 shell-scripts under perfmetrics/scripts. Do we have any reason to leave some of them?
E.g., ml_tests/pytorch/dino/setup_container.sh is not present in this PR.

@gargnitingoogle
Copy link
Collaborator Author

I can see there are 21 shell-scripts under perfmetrics/scripts. Do we have any reason to leave some of them? E.g., ml_tests/pytorch/dino/setup_container.sh is not present in this PR.

I haven't done it blanketly for all scripts, only for those scripts that were being chmoded somewhere or the other.

@raj-prince
Copy link
Collaborator

raj-prince commented Nov 7, 2023 •
edited by gargnitingoogle

I haven't done it blanketly for all scripts, only for those scripts that were being chmoded somewhere or the other.

Got it. Confused with the PR description Add execute mode to all shell scripts , we can update that.

Yeah, will change the title.

raj-prince
raj-prince previously approved these changes Nov 7, 2023
View reviewed changes
@gargnitingoogle
Copy link
Collaborator Author

The presubmit tests are all passing now. link

Perf results
+--------+------------+--------------+------------+--------------+--------------+
| Branch | File Size | Read BW | Write BW | RandRead BW | RandWrite BW |
+--------+------------+--------------+------------+--------------+--------------+
| Master | 0.25MiB | 505.41MiB/s | 1.04MiB/s | 35.39MiB/s | 1.08MiB/s |
| PR | 0.25MiB | 498.23MiB/s | 0.98MiB/s | 35.91MiB/s | 1.09MiB/s |
| | | | | | |
| | | | | | |
| Master | 48.828MiB | 4807.47MiB/s | 80.9MiB/s | 1219.08MiB/s | 79.66MiB/s |
| PR | 48.828MiB | 4787.23MiB/s | 73.52MiB/s | 1039.77MiB/s | 80.18MiB/s |
| | | | | | |
| | | | | | |
| Master | 976.562MiB | 4784.92MiB/s | 32.62MiB/s | 448.63MiB/s | 32.0MiB/s |
| PR | 976.562MiB | 4857.06MiB/s | 32.83MiB/s | 539.19MiB/s | 32.15MiB/s |
| | | | | | |
| | | | | | |
+--------+------------+--------------+------------+--------------+--------------+

@gargnitingoogle
Add execute mode to all shell scripts
f3ae29f 
This change adds execute mode (chmod +x)
to add script files in the gcsfuse project.
This is needed to avoid having to
`chmod +x` each time for running a shell
script especially a in jobs or in other scripts.
This `chmod +x` caused errors in running `git checkout`
in the calling script in the scenario like below.

```shell
get checkout master
chmod +x some_child_script.sh
get checkout dev_branch ### FAILS saying files are modified.
```

This change addresses internal bug#307698513 .
@gargnitingoogle
Add execute mode to more shell scripts
5c49d10
@gargnitingoogle
Add exec permission to more script files
8723021
@gargnitingoogle gargnitingoogle force-pushed the gargnitin-fix-script-git-checkout-error branch from 65de916 to d984fa3 Compare November 9, 2023 10:50
@gargnitingoogle
Copy link
Collaborator Author

gargnitingoogle commented Nov 9, 2023 •
edited

@ashmeenkaur @raj-prince I'm have added git reset --hard calls to avoid breaking presubmits in the meantime before the 2nd PR goes in. The 2nd PR undoes these git reset --hard calls.

Also earlier I had only added the exec-permissions to those script files, which were being chmoded somewhere in the code, but I've now extended the change to all the script files to match the PR title and description.

Please review and approve.

Once approving this, please also review follow-up PR #1489.

@gargnitingoogle gargnitingoogle force-pushed the gargnitin-fix-script-git-checkout-error branch from d984fa3 to 8723021 Compare November 10, 2023 08:02
@gargnitingoogle gargnitingoogle removed the execute-perf-test Execute performance test in PR label Nov 10, 2023
@gargnitingoogle gargnitingoogle merged commit 68bd98d into master Nov 10, 2023
8 checks passed
@gargnitingoogle gargnitingoogle deleted the gargnitin-fix-script-git-checkout-error branch November 10, 2023 11:18
gargnitingoogle added a commit that referenced this pull request Nov 28, 2023
@gargnitingoogle
Add exec-mode to all script files (#1486)
9bc8299 
* Add execute mode to all shell scripts

This change adds execute mode (chmod +x)
to add script files in the gcsfuse project.
This is needed to avoid having to
`chmod +x` each time for running a shell
script especially a in jobs or in other scripts.
This `chmod +x` caused errors in running `git checkout`
in the calling script in the scenario like below.

```shell
get checkout master
chmod +x some_child_script.sh
get checkout dev_branch ### FAILS saying files are modified.
```

This change addresses internal bug#307698513 .

* Add execute mode to more shell scripts

* Add exec permission to more script files
gargnitingoogle added a commit that referenced this pull request Nov 29, 2023
@gargnitingoogle
Add exec-mode to all script files (#1486)
1e75ab4 
* Add execute mode to all shell scripts

This change adds execute mode (chmod +x)
to add script files in the gcsfuse project.
This is needed to avoid having to
`chmod +x` each time for running a shell
script especially a in jobs or in other scripts.
This `chmod +x` caused errors in running `git checkout`
in the calling script in the scenario like below.

```shell
get checkout master
chmod +x some_child_script.sh
get checkout dev_branch ### FAILS saying files are modified.
```

This change addresses internal bug#307698513 .

* Add execute mode to more shell scripts

* Add exec permission to more script files
gargnitingoogle added a commit that referenced this pull request Nov 29, 2023
@gargnitingoogle
Add exec-mode to all script files (#1486)
f6359cf 
* Add execute mode to all shell scripts

This change adds execute mode (chmod +x)
to add script files in the gcsfuse project.
This is needed to avoid having to
`chmod +x` each time for running a shell
script especially a in jobs or in other scripts.
This `chmod +x` caused errors in running `git checkout`
in the calling script in the scenario like below.

```shell
get checkout master
chmod +x some_child_script.sh
get checkout dev_branch ### FAILS saying files are modified.
```

This change addresses internal bug#307698513 .

* Add execute mode to more shell scripts

* Add exec permission to more script files
@p-linnane p-linnane mentioned this pull request Dec 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raj-prince raj-prince raj-prince approved these changes

@ashmeenkaur ashmeenkaur ashmeenkaur approved these changes

@sethiay sethiay Awaiting requested review from sethiay

@vadlakondaswetha vadlakondaswetha Awaiting requested review from vadlakondaswetha

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gargnitingoogle @raj-prince @ashmeenkaur