You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/02/14 21:19:36 UTC
[GitHub] [incubator-nuttx] AlanRosenthal opened a new pull request #5496: Remove duplicate linker script definitions
AlanRosenthal opened a new pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496
## Summary
A lot of linker scripts were listed twice, once for unix, once for windows.
This PR cleans up the logic so they're only listed once.
## Impact
Any opportunity to use a single source of truth and reduce lines of code is a win!
## Testing
CI will test all build
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r807529757
##########
File path: arch/common/src/Makefile
##########
@@ -0,0 +1,27 @@
+############################################################################
+# arch/common/src/Makefile
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership. The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# Linker Scripts
+
+ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+LDFLAGS += $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
Review comment:
it's strange that the common folder just have one script file, how about we add a make function in Config.mk to extend the path with cygpath, and invoke that function like this:
```
LDFLAGS += $(addprefix -T $(xxx, $(ARCHSCRIPT)))
```
The new function can be used in many similar place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r806410593
##########
File path: arch/arm/src/Makefile
##########
@@ -19,6 +19,7 @@
############################################################################
include $(TOPDIR)/Make.defs
+include $(TOPDIR)/tools/Config.mk
Review comment:
don't need? (TOPDIR)/Make.defs will include Config.mk for us.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#issuecomment-1044768997
Please note this PR requires a change to all out-of-tree boards. I have added documentation about this to the upcoming Release Notes: [https://cwiki.apache.org/confluence/display/NUTTX/NuttX+10.3](https://cwiki.apache.org/confluence/display/NUTTX/NuttX+10.3). See section "Compatibility Concerns" ➡︎ "Changes to Build System"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r808269527
##########
File path: arch/common/src/Makefile
##########
@@ -0,0 +1,27 @@
+############################################################################
+# arch/common/src/Makefile
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership. The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# Linker Scripts
+
+ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+LDFLAGS += $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
Review comment:
Yes, how about we put the share script to either:
./tools/
./arch/
We can even invoke arch/Makefile from toplevel Makefile and then:
1. Define the common setting
2. Include the arch specific Makefile
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r808280595
##########
File path: arch/common/src/Makefile
##########
@@ -0,0 +1,27 @@
+############################################################################
+# arch/common/src/Makefile
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership. The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# Linker Scripts
+
+ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+LDFLAGS += $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
Review comment:
Yes, how about put the share script to:
./tools/
./arch/
we can even invoke arch/Makefile and let's arch/Makefile include the arch specific Makefile.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] AlanRosenthal commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
AlanRosenthal commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r806283845
##########
File path: tools/Config.mk
##########
@@ -548,3 +548,11 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# Linker Scripts
+
+ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+ ARCHSCRIPT_CLI = $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
+else
+ ARCHSCRIPT_CLI = $(addprefix -T,$(ARCHSCRIPT))
+endif
Review comment:
yeah, that should work. Thanks for the suggestion!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] AlanRosenthal commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
AlanRosenthal commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r806319304
##########
File path: tools/Config.mk
##########
@@ -548,3 +548,11 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# Linker Scripts
+
+ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+ ARCHSCRIPT_CLI = $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
+else
+ ARCHSCRIPT_CLI = $(addprefix -T,$(ARCHSCRIPT))
+endif
Review comment:
I noticed the flags were getting double added. I added a guard around it to prevent it from ever happening. Let me know what you think
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] AlanRosenthal commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
AlanRosenthal commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r806283845
##########
File path: tools/Config.mk
##########
@@ -548,3 +548,11 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# Linker Scripts
+
+ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+ ARCHSCRIPT_CLI = $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
+else
+ ARCHSCRIPT_CLI = $(addprefix -T,$(ARCHSCRIPT))
+endif
Review comment:
yeah, that should work. Thanks for the suggestion!
##########
File path: boards/arm/samv7/common/scripts/flat.memory
##########
@@ -22,10 +22,6 @@ include $(TOPDIR)/.config
include $(TOPDIR)/tools/Config.mk
include $(TOPDIR)/arch/arm/src/armv7-m/Toolchain.defs
-ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- ARCHSCRIPT = -T "${shell cygpath -w $(BOARD_COMMON_DIR)$(DELIM)scripts$(DELIM)samv7.ld}"
-else
- ARCHSCRIPT = -T$(BOARD_COMMON_DIR)$(DELIM)scripts$(DELIM)samv7.ld
-endif
+ARCHSCRIPT += $(BOARD_COMMON_DIR)$(DELIM)scripts$(DELIM)samv7.ld
Review comment:
added!
##########
File path: boards/xtensa/esp32/esp32-devkitc/scripts/Make.defs
##########
@@ -23,42 +23,34 @@ include $(TOPDIR)/tools/Config.mk
include $(TOPDIR)/tools/esp32/Config.mk
include $(TOPDIR)/arch/xtensa/src/lx6/Toolchain.defs
+# This is the generated memory layout linker script. It will always be
+# generated at the board level.
+
+ARCHSCRIPT += $(BOARD_DIR)$(DELIM)scripts$(DELIM)esp32_out.ld
Review comment:
reordered to maintain previous script order.
##########
File path: tools/Config.mk
##########
@@ -548,3 +548,11 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# Linker Scripts
+
+ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+ ARCHSCRIPT_CLI = $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
+else
+ ARCHSCRIPT_CLI = $(addprefix -T,$(ARCHSCRIPT))
+endif
Review comment:
I noticed the flags were getting double added. I added a guard around it to prevent it from ever happening. Let me know what you think
##########
File path: tools/Config.mk
##########
@@ -548,3 +548,17 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# Linker Scripts
+
+# Note: This file has the potenital to be included multiple times. To prevent duplicate linker scripts
+# from being added, guard the `LDFLAGS +=` with $(ARCHSCRIPT_ALREADY_ADDED)
+
+ifeq ($(ARCHSCRIPT_ALREADY_ADDED),)
+ ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+ LDFLAGS += $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
+ else
+ LDFLAGS += $(addprefix -T,$(ARCHSCRIPT))
+ endif
+ ARCHSCRIPT_ALREADY_ADDED = y
+endif
Review comment:
Instead of adding it in tools/Config.mk, I think a better spot might be `arch/common/src/Makefile`. This would create a spot for common code in arch/*/src/Makefile
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#issuecomment-1041908192
@AlanRosenthal could you squash the patchset, the change look good to me now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] AlanRosenthal commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
AlanRosenthal commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r808353186
##########
File path: arch/common/src/Makefile
##########
@@ -0,0 +1,27 @@
+############################################################################
+# arch/common/src/Makefile
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership. The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# Linker Scripts
+
+ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+LDFLAGS += $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
Review comment:
That's a good idea. I'll play around with it and see how it looks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r806762787
##########
File path: tools/Config.mk
##########
@@ -548,3 +548,17 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# Linker Scripts
+
+# Note: This file has the potenital to be included multiple times. To prevent duplicate linker scripts
+# from being added, guard the `LDFLAGS +=` with $(ARCHSCRIPT_ALREADY_ADDED)
+
+ifeq ($(ARCHSCRIPT_ALREADY_ADDED),)
+ ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+ LDFLAGS += $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
+ else
+ LDFLAGS += $(addprefix -T,$(ARCHSCRIPT))
+ endif
+ ARCHSCRIPT_ALREADY_ADDED = y
+endif
Review comment:
Do we know the reason why linker scripts are added multiple times? Is this consistent on both native Linux and Cygwin?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] AlanRosenthal commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
AlanRosenthal commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r806942914
##########
File path: tools/Config.mk
##########
@@ -548,3 +548,17 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# Linker Scripts
+
+# Note: This file has the potenital to be included multiple times. To prevent duplicate linker scripts
+# from being added, guard the `LDFLAGS +=` with $(ARCHSCRIPT_ALREADY_ADDED)
+
+ifeq ($(ARCHSCRIPT_ALREADY_ADDED),)
+ ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+ LDFLAGS += $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
+ else
+ LDFLAGS += $(addprefix -T,$(ARCHSCRIPT))
+ endif
+ ARCHSCRIPT_ALREADY_ADDED = y
+endif
Review comment:
Instead of adding it in tools/Config.mk, I think a better spot might be `arch/common/src/Makefile`. This would create a spot for common code in arch/*/src/Makefile
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r806270884
##########
File path: boards/arm/samv7/common/scripts/flat.memory
##########
@@ -22,10 +22,6 @@ include $(TOPDIR)/.config
include $(TOPDIR)/tools/Config.mk
include $(TOPDIR)/arch/arm/src/armv7-m/Toolchain.defs
-ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- ARCHSCRIPT = -T "${shell cygpath -w $(BOARD_COMMON_DIR)$(DELIM)scripts$(DELIM)samv7.ld}"
-else
- ARCHSCRIPT = -T$(BOARD_COMMON_DIR)$(DELIM)scripts$(DELIM)samv7.ld
-endif
+ARCHSCRIPT += $(BOARD_COMMON_DIR)$(DELIM)scripts$(DELIM)samv7.ld
Review comment:
What about `boards/arm/samv7/common/scripts/protected.memory` and other boards protected memory options?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#issuecomment-1042748569
LGTM!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r806410593
##########
File path: arch/arm/src/Makefile
##########
@@ -19,6 +19,7 @@
############################################################################
include $(TOPDIR)/Make.defs
+include $(TOPDIR)/tools/Config.mk
Review comment:
don't need? (TOPDIR)/Make.defs will include Config.mk for us.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] AlanRosenthal commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
AlanRosenthal commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r806303026
##########
File path: boards/arm/samv7/common/scripts/flat.memory
##########
@@ -22,10 +22,6 @@ include $(TOPDIR)/.config
include $(TOPDIR)/tools/Config.mk
include $(TOPDIR)/arch/arm/src/armv7-m/Toolchain.defs
-ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- ARCHSCRIPT = -T "${shell cygpath -w $(BOARD_COMMON_DIR)$(DELIM)scripts$(DELIM)samv7.ld}"
-else
- ARCHSCRIPT = -T$(BOARD_COMMON_DIR)$(DELIM)scripts$(DELIM)samv7.ld
-endif
+ARCHSCRIPT += $(BOARD_COMMON_DIR)$(DELIM)scripts$(DELIM)samv7.ld
Review comment:
added!
##########
File path: boards/xtensa/esp32/esp32-devkitc/scripts/Make.defs
##########
@@ -23,42 +23,34 @@ include $(TOPDIR)/tools/Config.mk
include $(TOPDIR)/tools/esp32/Config.mk
include $(TOPDIR)/arch/xtensa/src/lx6/Toolchain.defs
+# This is the generated memory layout linker script. It will always be
+# generated at the board level.
+
+ARCHSCRIPT += $(BOARD_DIR)$(DELIM)scripts$(DELIM)esp32_out.ld
Review comment:
reordered to maintain previous script order.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] AlanRosenthal commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
AlanRosenthal commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r808215207
##########
File path: arch/common/src/Makefile
##########
@@ -0,0 +1,27 @@
+############################################################################
+# arch/common/src/Makefile
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership. The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# Linker Scripts
+
+ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+LDFLAGS += $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
Review comment:
There's a lot of duplication between arch/*/src/Makefile. I think it would be nice to have a location to keep common code. The linker script would just be the first of many
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r806278895
##########
File path: tools/Config.mk
##########
@@ -548,3 +548,11 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# Linker Scripts
+
+ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+ ARCHSCRIPT_CLI = $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
+else
+ ARCHSCRIPT_CLI = $(addprefix -T,$(ARCHSCRIPT))
+endif
Review comment:
Is it possible just
```
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
LDFLAGS = $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
else
LDFLAGS = $(addprefix -T,$(ARCHSCRIPT))
endif
```
And drop `ARCHSCRIPT_CLI`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r806278895
##########
File path: tools/Config.mk
##########
@@ -548,3 +548,11 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# Linker Scripts
+
+ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+ ARCHSCRIPT_CLI = $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
+else
+ ARCHSCRIPT_CLI = $(addprefix -T,$(ARCHSCRIPT))
+endif
Review comment:
Is it possible just
```
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
LDFLAGS += $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
else
LDFLAGS += $(addprefix -T,$(ARCHSCRIPT))
endif
```
And drop `ARCHSCRIPT_CLI`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r806280305
##########
File path: arch/sim/src/Makefile
##########
@@ -294,8 +295,8 @@ ifeq ("$(shell $(CC) --version | grep clang)","")
$(Q) echo "__init_array_start = .; __init_array_end = .; __fini_array_start = .; __fini_array_end = .;" >>nuttx.ld
endif
$(if $(CONFIG_HAVE_CXX),\
- $(Q) "$(CXX)" $(CCLINKFLAGS) $(LIBPATHS) $(ARCHSCRIPT) -o $(TOPDIR)/$@ $(HEADOBJ) nuttx.rel $(HOSTOBJS) $(STDLIBS),\
- $(Q) "$(CC)" $(CCLINKFLAGS) $(LIBPATHS) $(ARCHSCRIPT) -o $(TOPDIR)/$@ $(HEADOBJ) nuttx.rel $(HOSTOBJS) $(STDLIBS))
+ $(Q) "$(CXX)" $(CCLINKFLAGS) $(LIBPATHS) $(ARCHSCRIPT_CLI) -o $(TOPDIR)/$@ $(HEADOBJ) nuttx.rel $(HOSTOBJS) $(STDLIBS),\
Review comment:
If we can use `LDFLAGS` in `tools/Config.mk` then we can `$(Q) "$(CXX)" $(CCLINKFLAGS) $(LIBPATHS) $(LDFLAGS) -o $(TOPDIR)/$@ $(HEADOBJ) nuttx.rel $(HOSTOBJS) $(STDLIBS),\` here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r806270884
##########
File path: boards/arm/samv7/common/scripts/flat.memory
##########
@@ -22,10 +22,6 @@ include $(TOPDIR)/.config
include $(TOPDIR)/tools/Config.mk
include $(TOPDIR)/arch/arm/src/armv7-m/Toolchain.defs
-ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- ARCHSCRIPT = -T "${shell cygpath -w $(BOARD_COMMON_DIR)$(DELIM)scripts$(DELIM)samv7.ld}"
-else
- ARCHSCRIPT = -T$(BOARD_COMMON_DIR)$(DELIM)scripts$(DELIM)samv7.ld
-endif
+ARCHSCRIPT += $(BOARD_COMMON_DIR)$(DELIM)scripts$(DELIM)samv7.ld
Review comment:
What about `boards/arm/samv7/common/scripts/protected.memory` and other boards protected memory options?
##########
File path: tools/Config.mk
##########
@@ -548,3 +548,11 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# Linker Scripts
+
+ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+ ARCHSCRIPT_CLI = $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
+else
+ ARCHSCRIPT_CLI = $(addprefix -T,$(ARCHSCRIPT))
+endif
Review comment:
Is it possible just
```
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
LDFLAGS = $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
else
LDFLAGS = $(addprefix -T,$(ARCHSCRIPT))
endif
```
And drop `ARCHSCRIPT_CLI`?
##########
File path: tools/Config.mk
##########
@@ -548,3 +548,11 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# Linker Scripts
+
+ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+ ARCHSCRIPT_CLI = $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
+else
+ ARCHSCRIPT_CLI = $(addprefix -T,$(ARCHSCRIPT))
+endif
Review comment:
Is it possible just
```
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
LDFLAGS += $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
else
LDFLAGS += $(addprefix -T,$(ARCHSCRIPT))
endif
```
And drop `ARCHSCRIPT_CLI`?
##########
File path: arch/sim/src/Makefile
##########
@@ -294,8 +295,8 @@ ifeq ("$(shell $(CC) --version | grep clang)","")
$(Q) echo "__init_array_start = .; __init_array_end = .; __fini_array_start = .; __fini_array_end = .;" >>nuttx.ld
endif
$(if $(CONFIG_HAVE_CXX),\
- $(Q) "$(CXX)" $(CCLINKFLAGS) $(LIBPATHS) $(ARCHSCRIPT) -o $(TOPDIR)/$@ $(HEADOBJ) nuttx.rel $(HOSTOBJS) $(STDLIBS),\
- $(Q) "$(CC)" $(CCLINKFLAGS) $(LIBPATHS) $(ARCHSCRIPT) -o $(TOPDIR)/$@ $(HEADOBJ) nuttx.rel $(HOSTOBJS) $(STDLIBS))
+ $(Q) "$(CXX)" $(CCLINKFLAGS) $(LIBPATHS) $(ARCHSCRIPT_CLI) -o $(TOPDIR)/$@ $(HEADOBJ) nuttx.rel $(HOSTOBJS) $(STDLIBS),\
Review comment:
If we can use `LDFLAGS` in `tools/Config.mk` then we can `$(Q) "$(CXX)" $(CCLINKFLAGS) $(LIBPATHS) $(LDFLAGS) -o $(TOPDIR)/$@ $(HEADOBJ) nuttx.rel $(HOSTOBJS) $(STDLIBS),\` here
##########
File path: tools/Config.mk
##########
@@ -548,3 +548,17 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# Linker Scripts
+
+# Note: This file has the potenital to be included multiple times. To prevent duplicate linker scripts
+# from being added, guard the `LDFLAGS +=` with $(ARCHSCRIPT_ALREADY_ADDED)
+
+ifeq ($(ARCHSCRIPT_ALREADY_ADDED),)
+ ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+ LDFLAGS += $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
+ else
+ LDFLAGS += $(addprefix -T,$(ARCHSCRIPT))
+ endif
+ ARCHSCRIPT_ALREADY_ADDED = y
+endif
Review comment:
Do we know the reason why linker scripts are added multiple times? Is this consistent on both native Linux and Cygwin?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5496: Remove duplicate linker script definitions
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5496:
URL: https://github.com/apache/incubator-nuttx/pull/5496#discussion_r807529757
##########
File path: arch/common/src/Makefile
##########
@@ -0,0 +1,27 @@
+############################################################################
+# arch/common/src/Makefile
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership. The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# Linker Scripts
+
+ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
+LDFLAGS += $(addprefix -T,$(foreach SCRIPT,$(ARCHSCRIPT),${shell cygpath -w $(SCRIPT)}))
Review comment:
it's strange that the common folder just have one script file, how about we add a make function in Config.mk to extend the path with cygpath conditionally, and invoke that function like this:
```
LDFLAGS += $(addprefix -T $(xxx, $(ARCHSCRIPT)))
```
The new function can be used in many similar place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org