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

Fix Makefile to Support Spaces in Paths #4000

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

EthanDieterich
Copy link

Overview

In this branch, I fixed issues my team and I encountered with the Makefile, where it would not function correctly when spaces were present in the path. The issue was traced back to how PATH and SHELL were being set, leading to failures when running make in directories with spaces in their names.

What this PR does / why we need it

  • This PR ensures that paths are properly quoted in the Makefile to prevent issues caused by spaces in directory names.
  • Updates how SHELL passes the PATH variable to ensure compatibility with /bin/sh while keeping the correct environment settings.
  • This fix is necessary for users working in directories with spaces (e.g., /home/user/My Project/), where make previously failed due to improper path handling.

Special notes for your reviewer

@EthanDieterich EthanDieterich marked this pull request as ready for review February 20, 2025 21:06
@jsquyres
Copy link

Can this PR be reviewed? We have found that we need this for environments that have a PATH with a space.

Thanks!

@sagikazarmark
Copy link
Member

I wonder if this change is the reason why tests are failing

@jsquyres
Copy link

I wonder if this change is the reason why tests are failing

Interesting:

/bin/sh: 1: protoc: Permission denied

Does this mean that make verify in the github action was previously finding a different protoc (that properly has the x bit set)?

I'm guessing that we also need:

diff --git a/Makefile b/Makefile
index 3cc72405..f189183b 100644
--- a/Makefile
+++ b/Makefile
@@ -87,6 +87,7 @@ ifeq ($(shell uname | tr A-Z a-z), linux)
        curl -L https://github.com/protocolbuffers/protobuf/releases/download/v>
 endif
        unzip bin/protoc.zip -d bin/protoc
+       chmod +x bin/protoc
        rm bin/protoc.zip
 
 bin/protoc-gen-go:

For the second CI failure:

#89 0.177 /bin/sh: uname: not found
#89 0.181 /bin/sh: uname: not found

I'm not sure if that's the actual underlying error, but I do see that in the Github Action log. I'm assuming it's from the various calls to $(shell uname) in the Makefile.

But offhand, I can't think of why adding the quotes to the PATH would make uname suddenly be unable to be found...?

Oh, actually, I see this in the log output, too:

#89 9.400 env: can't execute 'sh': No such file or directory

Odd.

@jsquyres
Copy link

I'm guessing that we also need:

I added a commit to this PR that added the chmod +x .... Could you approve the github actions again to see if that problem at least got fixed?

I haven't had time to dig into the 2nd failure yet, but I offhand wonder if that failed because protoc previously didn't run...?

@jsquyres jsquyres force-pushed the fix-makefile-path branch from 2fe2847 to 326d2bd Compare March 1, 2025 00:04
@jsquyres
Copy link

jsquyres commented Mar 1, 2025

Oops -- had the wrong chmod path in there. Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants