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 outputs path (append /bin) #92

Conversation

sebastienvermeille
Copy link
Contributor

@sebastienvermeille sebastienvermeille commented Oct 4, 2023

Fix outputs path (append /bin)

Changes

  • Append /bin to the output.path
    That way it provides a path to a dir which contains protoc executable as expected

The current path value provided by arduino/setup-protoc action requires me to do that workaround:
https://github.com/sebastienvermeille/another-protobuf-maven-plugin/blob/master/.github/workflows/build.yml#L55

I believe it makes sense to have the /bin included so that we benefit directly access to the binaries of protoc.

Note: please before merging this PR could you please add the label HACKTOBERFEST-ACCEPTED to this PR ? That would be greatly appreciated :) Thank's a lot

That way it provides a path to a dir which contains protoc executable as expected
@sebastienvermeille sebastienvermeille marked this pull request as ready for review October 4, 2023 06:55
@per1234 per1234 added type: imperfection Perceived defect in any part of project topic: code Related to content of the project itself labels Oct 4, 2023
@alessio-perugini
Copy link
Contributor

@sebastienvermeille 👋 I think appending the /bin in the current path variable is not a good idea for the following reasons:

  1. It would be a breaking change for all the users that rely on the output path variable
  2. When we extract the protoc archive, it contains 2 folders: bin and include. Although I think it is very rare, it might be that someone wants to have access to the include folder.

We also install the protoc in the PATH env. You should be able to directly call the protoc without any issues.

An option, could be to ship a new output variable called binary containing the full path. (binary=/some/path/bin/protoc) But I'm not convinced about that since we already have 2 ways of accessing that binary.

@sebastienvermeille
Copy link
Contributor Author

Hi @alessio-perugini ok let's keep it like that then 👍

@sebastienvermeille sebastienvermeille deleted the svermeille/fix-output-path branch October 4, 2023 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants