Skip to content

chore: fix broken make proto#235

Merged
kohlisid merged 2 commits intonumaproj:mainfrom
tmenjo:issues/234/fix-broken-make-proto
Jul 22, 2025
Merged

chore: fix broken make proto#235
kohlisid merged 2 commits intonumaproj:mainfrom
tmenjo:issues/234/fix-broken-make-proto

Conversation

@tmenjo
Copy link
Copy Markdown
Contributor

@tmenjo tmenjo commented Jul 11, 2025

Close #234

Signed-off-by: Takashi Menjo <takashi.menjo+github@gmail.com>
@tmenjo
Copy link
Copy Markdown
Contributor Author

tmenjo commented Jul 11, 2025

Note that, without change of the sed line, I got a No such file or directory error as follows:

$ make proto
poetry run python3 -m grpc_tools.protoc --pyi_out=pynumaflow/proto/sinker -I=pynumaflow/proto/sinker --python_out=pynumaflow/proto/sinker --grpc_python_out=pynumaflow/proto/sinker  pynumaflow/proto/sinker/*.proto
poetry run python3 -m grpc_tools.protoc --pyi_out=pynumaflow/proto/mapper -I=pynumaflow/proto/mapper --python_out=pynumaflow/proto/mapper --grpc_python_out=pynumaflow/proto/mapper  pynumaflow/proto/mapper/*.proto
poetry run python3 -m grpc_tools.protoc --pyi_out=pynumaflow/proto/reducer -I=pynumaflow/proto/reducer --python_out=pynumaflow/proto/reducer --grpc_python_out=pynumaflow/proto/reducer  pynumaflow/proto/reducer/*.proto
poetry run python3 -m grpc_tools.protoc --pyi_out=pynumaflow/proto/sourcetransformer -I=pynumaflow/proto/sourcetransformer --python_out=pynumaflow/proto/sourcetransformer --grpc_python_out=pynumaflow/proto/sourcetransformer  pynumaflow/proto/sourcetransformer/*.proto
poetry run python3 -m grpc_tools.protoc --pyi_out=pynumaflow/proto/sideinput -I=pynumaflow/proto/sideinput --python_out=pynumaflow/proto/sideinput --grpc_python_out=pynumaflow/proto/sideinput  pynumaflow/proto/sideinput/*.proto
poetry run python3 -m grpc_tools.protoc --pyi_out=pynumaflow/proto/sourcer -I=pynumaflow/proto/sourcer --python_out=pynumaflow/proto/sourcer --grpc_python_out=pynumaflow/proto/sourcer  pynumaflow/proto/sourcer/*.proto
sed -i '' 's/^\(import.*_pb2\)/from . \1/' pynumaflow/proto/*/*.py
sed: can't read s/^\(import.*_pb2\)/from . \1/: No such file or directory
make: *** [Makefile:35: proto] Error 2

@tmenjo tmenjo marked this pull request as ready for review July 11, 2025 00:21
@tmenjo tmenjo requested review from ab93, vigith and whynowy as code owners July 11, 2025 00:21
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.26%. Comparing base (9a390a5) to head (f257264).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #235   +/-   ##
=======================================
  Coverage   94.26%   94.26%           
=======================================
  Files          60       60           
  Lines        2441     2441           
  Branches      124      124           
=======================================
  Hits         2301     2301           
  Misses        101      101           
  Partials       39       39           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tmenjo
Copy link
Copy Markdown
Contributor Author

tmenjo commented Jul 11, 2025

Note that, without change of the sed line, I got a No such file or directory error as follows:

I just noticed that an empty string argument for sed -i option is probably for sed on Mac. I made and checked this PR on Ubuntu 24.04 and GNU sed.

Can anyone here try to run make proto with this PR on Mac?

Signed-off-by: Takashi Menjo <takashi.menjo+github@gmail.com>
@tmenjo
Copy link
Copy Markdown
Contributor Author

tmenjo commented Jul 11, 2025

I added a commit that sed in make proto can run both on Linux and Mac: give -i.bak option to sed to preserve current files with .bak extension then rm all of them.

@vigith
Copy link
Copy Markdown
Member

vigith commented Jul 11, 2025

I added a commit that sed in make proto can run both on Linux and Mac: give -i.bak option to sed to preserve current files with .bak extension then rm all of them.

I don't think we need to use the -i option. Any reason you want to use it?

@tmenjo
Copy link
Copy Markdown
Contributor Author

tmenjo commented Jul 11, 2025

I don't think we need to use the -i option. Any reason you want to use it?

Without -i option, the results of sed are output to stdout, and given files (pynumaflow/proto/*/*.py) remain unchanged.

@vigith
Copy link
Copy Markdown
Member

vigith commented Jul 11, 2025

sorry, my bad, why do we need to take the backup? I asked the wrong question

@tmenjo
Copy link
Copy Markdown
Contributor Author

tmenjo commented Jul 11, 2025

why do we need to take the backup?

We don't need backup actually, but I have no choice but to use sed -i.bak (then rm all .bak files) because I want our make proto work on both Linux and Mac.

It's a compatibility issue between GNU sed (Linux) and BSD sed (Mac). Googling with [sed i linux mac], we can find many pages about the issue such as [1].

If I just think about Linux (my dev env using GNU sed), my sed command line can be as follows with no backup. However, it possibly doesn't work properly on Mac (I haven't tried actually since I have no Mac):

sed -i -e 's/^\(import.*_pb2\)/from . \1/' pynumaflow/proto/*/*.py

In contrast to that, the following command line having an empty string for -i option may work on Mac, but doesn't on Linux (I got sed: can't read : No such file or directory):

sed -i '' -e 's/^\(import.*_pb2\)/from . \1/' pynumaflow/proto/*/*.py

[1] https://stackoverflow.com/questions/4247068/sed-command-with-i-option-failing-on-mac-but-works-on-linux

@kohlisid
Copy link
Copy Markdown
Contributor

(venv) skohli@macos-JQWR9T560R numaflow-python % make proto
poetry run python3 -m grpc_tools.protoc --pyi_out=pynumaflow/proto/sinker -I=pynumaflow/proto/sinker --python_out=pynumaflow/proto/sinker --grpc_python_out=pynumaflow/proto/sinker  pynumaflow/proto/sinker/*.proto
poetry run python3 -m grpc_tools.protoc --pyi_out=pynumaflow/proto/mapper -I=pynumaflow/proto/mapper --python_out=pynumaflow/proto/mapper --grpc_python_out=pynumaflow/proto/mapper  pynumaflow/proto/mapper/*.proto
poetry run python3 -m grpc_tools.protoc --pyi_out=pynumaflow/proto/reducer -I=pynumaflow/proto/reducer --python_out=pynumaflow/proto/reducer --grpc_python_out=pynumaflow/proto/reducer  pynumaflow/proto/reducer/*.proto
poetry run python3 -m grpc_tools.protoc --pyi_out=pynumaflow/proto/sourcetransformer -I=pynumaflow/proto/sourcetransformer --python_out=pynumaflow/proto/sourcetransformer --grpc_python_out=pynumaflow/proto/sourcetransformer  pynumaflow/proto/sourcetransformer/*.proto
poetry run python3 -m grpc_tools.protoc --pyi_out=pynumaflow/proto/sideinput -I=pynumaflow/proto/sideinput --python_out=pynumaflow/proto/sideinput --grpc_python_out=pynumaflow/proto/sideinput  pynumaflow/proto/sideinput/*.proto
poetry run python3 -m grpc_tools.protoc --pyi_out=pynumaflow/proto/sourcer -I=pynumaflow/proto/sourcer --python_out=pynumaflow/proto/sourcer --grpc_python_out=pynumaflow/proto/sourcer  pynumaflow/proto/sourcer/*.proto
sed -i.bak -e 's/^\(import.*_pb2\)/from . \1/' pynumaflow/proto/*/*.py
rm pynumaflow/proto/*/*.py.bak
(venv) skohli@macos-JQWR9T560R numaflow-python % git status

@kohlisid kohlisid merged commit 7faf39b into numaproj:main Jul 22, 2025
11 checks passed
@kohlisid
Copy link
Copy Markdown
Contributor

Thanks for the changes @tmenjo :D

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.

make proto looks broken

3 participants