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

Rename make folder to get rid of build error. #12880

Merged
merged 1 commit into from Jun 14, 2023

Conversation

avida
Copy link
Contributor

@avida avida commented Jun 10, 2023

When trying to build firmware with current directory in PATH environment it scans for make command and generates "Permission denied" error in case if current directory in PATH precedes /usr/bin/ directory.In my case it was caused by incorrect pyenv init script.

Rename make folder to avoid errors like this.

@github-actions
Copy link

Do you want to test this code? You can flash it directly from Betaflight Configurator:

  • Simply put #12880 (this pull request number) in the Select commit field of the Configurator firmware flasher tab (you need to Enable expert mode, Show release candidates and Development).

WARNING: It may be unstable. Use only for testing!

@blckmn
Copy link
Member

blckmn commented Jun 10, 2023

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> PASS
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> FAIL
  • Tested label found -> FAIL
  • assigned to an approver -> FAIL
  • approver count at least three -> PASS

Copy link
Member

@blckmn blckmn left a comment

Choose a reason for hiding this comment

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

Could you please rename this directory to mk as all the others use shortened names. e.g. src, lib etc

@avida
Copy link
Contributor Author

avida commented Jun 11, 2023

Could you please rename this directory to mk as all the others use shortened names. e.g. src, lib etc

Done.

@ledvinap
Copy link
Contributor

@avida what system are you using?

@avida
Copy link
Contributor Author

avida commented Jun 11, 2023

@avida what system are you using?

Linux Mint, pyenv 2.3.19

@haslinghuis haslinghuis added this to the 4.5 milestone Jun 11, 2023
haslinghuis
haslinghuis previously approved these changes Jun 11, 2023
@ledvinap
Copy link
Contributor

@avida : Well, it seems to be bug in GNU make ..

@ledvinap
Copy link
Contributor

Bug is already fixed in coreutil (and make). coreutils/gnulib@6e6abd0

@ledvinap
Copy link
Contributor

The MAKE_SCRIPT_DIR is definitely good idea. But rename make -> mk may not be necessary. make bug is already fixed (but it will take some time; bug was fixed in 2020). Maybe just document this behavior and assume users are using sane build environment.

@haslinghuis haslinghuis dismissed their stale review June 12, 2023 14:24

Already fixed?

@blckmn
Copy link
Member

blckmn commented Jun 14, 2023

The MAKE_SCRIPT_DIR is definitely good idea. But rename make -> mk may not be necessary. make bug is already fixed (but it will take some time; bug was fixed in 2020). Maybe just document this behavior and assume users are using sane build environment.

I am ok with the rename. it shouldn't cause any issues (aside from rebases required if there are PRs open).

src/test/Makefile Outdated Show resolved Hide resolved
src/test/Makefile Outdated Show resolved Hide resolved
src/test/Makefile Outdated Show resolved Hide resolved
When trying to build firmware with current directory in PATH environment
it scans for make command and generates "Permission denied" error in
case if current directory in PATH precedes /usr/bin/ directory.In my
case it was caused by incorrect pyenv init script.

Rename make folder to avoid errors like this.
Copy link
Member

@haslinghuis haslinghuis left a comment

Choose a reason for hiding this comment

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

Thanks.

@haslinghuis haslinghuis merged commit 7b39d3d into betaflight:master Jun 14, 2023
20 checks passed
@hydra
Copy link
Contributor

hydra commented Jul 12, 2023

Just ran into this today, wondering why my make/local.mk was not picked up.

I disagree with this change. The principle behind the project structure that was setup in Cleanflight 10 years ago was so that newcomers to the project can easily navigate the project and not have to look in folders without being able to guess the content of the folder before opening it.

mk is an abbreviation, if you don't know about make using .mk as a file suffix you've got no idea what the heck is in a folder called mk.

make on the other hand is an English work that carries meaning, you can guess that there's stuff to do with making artifacts.

Changing the folder names as a workaround to an incorrectly setup environment, IMHO, is not the answer.

Please can we revert this?

haslinghuis added a commit that referenced this pull request Jul 12, 2023
@blckmn
Copy link
Member

blckmn commented Jul 13, 2023

In a similar vane... you would presume that src should be source? or is there a lack of consistency in the approach?

@hydra
Copy link
Contributor

hydra commented Jul 13, 2023

In a similar vane... you would presume that src should be source? or is there a lack of consistency in the approach?

it's true, but that's because at the time I was coding a lot in Java and Groovy where 'src' is a common abbreviation

however, 'src' is a common abbreviation across ALL programming languages, whereas 'mk' is not, and the point still stands that using 'mk' to avoid a user-specific environment issue is a bad idea when for 10+ years using 'make' has been 100% fine for every developer so far.

@blckmn
Copy link
Member

blckmn commented Jul 14, 2023

obj (for objects), bin (for binaries) ... the point is, shortening directory names is not uncommon and has been in practice for considerable time.

The fact is the make directory wasn't even introduced as part of cleanflight. It was Nathan Soi who introduced it 7 years ago (to add the arm_sdk_install target), and it was then further enhanced by myself and other contributors in betaflight.

I am not overly fussed to be honest - it can be renamed to make to satisfy your annoyance at the change - but let's not rewrite history.

@hydra
Copy link
Contributor

hydra commented Jul 14, 2023

The fact is the make directory wasn't even introduced as part of cleanflight. It was Nathan Soi who introduced it 7 years ago (to add the arm_sdk_install target), and it was then further enhanced by myself and other contributors in betaflight.

I am not overly fussed to be honest - it can be renamed to make to satisfy your annoyance at the change - but let's not rewrite history.

My bad, I'm mis-remembering it's origin. Carry on!

davidbitton pushed a commit to davidbitton/betaflight that referenced this pull request Feb 5, 2024
When trying to build firmware with current directory in PATH environment
it scans for make command and generates "Permission denied" error in
case if current directory in PATH precedes /usr/bin/ directory.In my
case it was caused by incorrect pyenv init script.

Rename make folder to avoid errors like this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: COMPLETED
Development

Successfully merging this pull request may close these issues.

None yet

5 participants