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

feat(options): add SRC_DIR var to allow sources to live in a subfolder #406

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mgcrea
Copy link

@mgcrea mgcrea commented Jan 30, 2016

Would fix #267.

@simonvanderveldt
Copy link

+1 for this, having a Makefile in the project's root and the project's source in the src dir is a pretty common thing for C projects.

@sudar What's needed for this to be merged? And can you trigger Travis again? Seems like it got stuck on something unrelated.

@sudar
Copy link
Owner

sudar commented Jun 13, 2016

@sej7278 What are your thoughts on this?

@simonvanderveldt

I don't see a way to restart the Travis test again. Let me see if I can manually trigger it somehow.

@sej7278
Copy link
Collaborator

sej7278 commented Jun 13, 2016

i know the ide is going to support searching the src/ subdirectory in 1.6.10 instead of recursively scanning all subdirectories that it currently does in 1.6.9 - which breaks cached rebuilds and you have to make clean every time! see arduino/arduino-builder#89

could we append rather than overwrite the variables, so we can use the current directory as well as the src/ subdirectory? as it is if SRC_DIR is defined you have to use it exclusively.

i'd rather we followed the ide and hardcoded SRC_DIR as "src/", like libraries do or at least initially define it as src/ e.g. $SRC_DIR ?= src

-LOCAL_C_SRCS    ?= $(wildcard *.c)
-LOCAL_CPP_SRCS  ?= $(wildcard *.cpp)
-LOCAL_CC_SRCS   ?= $(wildcard *.cc)
-LOCAL_PDE_SRCS  ?= $(wildcard *.pde)
-LOCAL_INO_SRCS  ?= $(wildcard *.ino)
-LOCAL_AS_SRCS   ?= $(wildcard *.S)
+SRC_DIR         ?= src
+LOCAL_C_SRCS    ?= $(wildcard $(SRC_DIR)/*.c *.c)
+LOCAL_CPP_SRCS  ?= $(wildcard $(SRC_DIR)/*.cpp *.cpp)
+LOCAL_CC_SRCS   ?= $(wildcard $(SRC_DIR)/*.cc *.cc)
+LOCAL_PDE_SRCS  ?= $(wildcard $(SRC_DIR)/*.pde *.pde)
+LOCAL_INO_SRCS  ?= $(wildcard $(SRC_DIR)/*.ino *.ino)
+LOCAL_AS_SRCS   ?= $(wildcard $(SRC_DIR)/*.S *.S)

by using USER_LIB_PATH we can also have libraries in the subdirectory too, this works:

BOARD_TAG = leonardo
MONITOR_PORT = /dev/ttyACM0
ARDUINO_LIBS = WiiChuck Wire Servo
USER_LIB_PATH = src/libs

include $(HOME)/Arduino-Makefile/Arduino.mk
├── Makefile
└── src
    ├── libs
    │   └── WiiChuck
    ├── pindefs.h
    └── wiichuck_buttons.ino

need to add some info to HISTORY.md and arduino-mk-vars.md

pip keeps breaking travis for some reason, temp files not being cleared or something.

@simonvanderveldt
Copy link

could we append rather than overwrite the variables, so we can use the current directory as well as the src/ subdirectory? as it is if SRC_DIR is defined you have to use it exclusively.

But isn't only using SRC_DIR for your sources the thing you'd want to achieve when you set it?
At least for me it is, the "magic" of also including other source dirs than the one I explicitly set as a user would just make unintentional files being included a bigger possibility.

@sej7278
Copy link
Collaborator

sej7278 commented Jun 14, 2016

yes its a good point perhaps its better to explicitly set the SRC_DIR (or not) rather than also include files from the CWD that may confuse things. you'd have to document that you need a trailing /

@sej7278
Copy link
Collaborator

sej7278 commented Jun 14, 2016

we should probably output SRC_DIR if its set to the Arduino.mk configuration screen like:

Arduino.mk Configuration:
...
- [USER]               SRC_DIR = src/ 
diff --git a/Arduino.mk b/Arduino.mk
index 322ca38..7957a34 100644
--- a/Arduino.mk
+++ b/Arduino.mk
@@ -762,12 +762,12 @@ endif
 ########################################################################
 # Local sources

-LOCAL_C_SRCS    ?= $(wildcard *.c)
-LOCAL_CPP_SRCS  ?= $(wildcard *.cpp)
-LOCAL_CC_SRCS   ?= $(wildcard *.cc)
-LOCAL_PDE_SRCS  ?= $(wildcard *.pde)
-LOCAL_INO_SRCS  ?= $(wildcard *.ino)
-LOCAL_AS_SRCS   ?= $(wildcard *.S)
+LOCAL_C_SRCS    ?= $(wildcard $(SRC_DIR)*.c)
+LOCAL_CPP_SRCS  ?= $(wildcard $(SRC_DIR)*.cpp)
+LOCAL_CC_SRCS   ?= $(wildcard $(SRC_DIR)*.cc)
+LOCAL_PDE_SRCS  ?= $(wildcard $(SRC_DIR)*.pde)
+LOCAL_INO_SRCS  ?= $(wildcard $(SRC_DIR)*.ino)
+LOCAL_AS_SRCS   ?= $(wildcard $(SRC_DIR)*.S)
 LOCAL_SRCS      = $(LOCAL_C_SRCS)   $(LOCAL_CPP_SRCS) \
                $(LOCAL_CC_SRCS)   $(LOCAL_PDE_SRCS) \
                $(LOCAL_INO_SRCS) $(LOCAL_AS_SRCS)
@@ -780,6 +780,10 @@ ifeq ($(words $(LOCAL_SRCS)), 0)
     $(error At least one source file (*.ino, *.pde, *.cpp, *c, *cc, *.S) is needed)
 endif

+ifdef SRC_DIR
+    $(call show_config_variable,SRC_DIR,[USER])
+endif
+
 # CHK_SOURCES is used by flymake
 # flymake creates a tmp file in the same directory as the file under edition
 # we must skip the verification in this particular case

Might be worth merging #429 into this commit too, as its the same area of code....?

@simonvanderveldt
Copy link

simonvanderveldt commented Jun 14, 2016

we should probably output SRC_DIR if its set to the Arduino.mk configuration screen like:

👍

Might be worth merging #429 into this commit too, as its the same area of code....?

Even though that PR touches parts of the same code there doesn't seem to be a real relation between these 2 issues/PRs and there's value in both of them separate from the other one, so I'd leave them as separate PRs.

@sej7278
Copy link
Collaborator

sej7278 commented Jun 14, 2016

i just meant this is going to totally break that PR, so maybe it would be nice to figure out a way around that. otherwise if we can get SRC_DIR documented and HISTORY.md updated, it should be good to merge i guess.

@simonvanderveldt
Copy link

i just meant this is going to totally break that PR,

Well, one of them has to rebase anyway ;)

@sudar
Copy link
Owner

sudar commented Sep 30, 2018

@sej7278

Do you still think this PR makes sense?

If yes, then I can update the Readme and documentation and trigger the Travis CI again.

If not, we can close it.

@sej7278
Copy link
Collaborator

sej7278 commented Sep 30, 2018

its a nice idea, and it looks like it should merge ok 👍

@sudar
Copy link
Owner

sudar commented Oct 7, 2018

@mgcrea @simonvanderveldt

Can you please add a note about the changes in the History.md file? We would be happy to merge it once we have the updated History.md file.

@@ -762,12 +762,12 @@ endif
########################################################################
# Local sources

Choose a reason for hiding this comment

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

Excited to see this!

May I suggest:

Suggested change
SRC_DIR ?= $(dir $(realpath $(firstword $(MAKEFILE_LIST))))

This would default SRC_DIR to the location of the users Makefile. Since it would then always be defined, you could then print it in configuration vars without it being weird and use slash notation in your wildcard searches (if you cared to).

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.

Ability to put src files in a subfolder
5 participants