ppisa commented on code in PR #3624: URL: https://github.com/apache/nuttx-apps/pull/3624#discussion_r3655027876
########## graphics/microwindows/Makefile: ########## @@ -0,0 +1,142 @@ +############################################################################ +# apps/graphics/microwindows/Makefile +# +# SPDX-License-Identifier: Apache-2.0 +# +# 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. +# +############################################################################ + +include $(APPDIR)/Make.defs + +# Microwindows graphic library + +MICROWINDOWS_DIR = . +MICROWINDOWS_DIR_NAME = microwindows + +# Set up build configuration and environment + +WD := ${shell echo $(CURDIR) | sed -e 's/ /\\ /g'} + +MICROWINDOWS_COMMIT_HASH := 1f269127b991d01dea5784eacee321a23e34d3d5 + +CONFIG_GRAPH_MICROWINDOWS_URL ?= https://codeload.github.com/ghaerr/microwindows/zip/$(MICROWINDOWS_COMMIT_HASH) + +MICROWINDOWS_TARBALL := microwindows-$(MICROWINDOWS_COMMIT_HASH).zip + +MICROWINDOWS_UNPACKNAME = microwindows +UNPACK ?= unzip -o $(if $(V),,-q) +CURL ?= curl -L $(if $(V),,-Ss) + +MICROWINDOWS_UNPACKDIR = $(WD)/$(MICROWINDOWS_UNPACKNAME) + +$(MICROWINDOWS_TARBALL): + $(ECHO_BEGIN)"Downloading: $(MICROWINDOWS_TARBALL)" + $(Q) $(CURL) -o $(MICROWINDOWS_TARBALL) $(CONFIG_GRAPH_MICROWINDOWS_URL) + $(ECHO_END) + +$(MICROWINDOWS_UNPACKNAME): $(MICROWINDOWS_TARBALL) + $(ECHO_BEGIN)"Unpacking: $(MICROWINDOWS_TARBALL) -> $(MICROWINDOWS_UNPACKNAME)" + $(Q) $(UNPACK) $(MICROWINDOWS_TARBALL) + $(Q) mv microwindows-$(MICROWINDOWS_COMMIT_HASH) $(MICROWINDOWS_UNPACKNAME) + $(Q) touch $(MICROWINDOWS_UNPACKNAME) + $(ECHO_END) + +# Download and unpack tarball if no git repo found +ifeq ($(wildcard $(MICROWINDOWS_UNPACKNAME)/.git),) +context:: $(MICROWINDOWS_UNPACKNAME) + +distclean:: + $(call DELDIR, $(MICROWINDOWS_UNPACKNAME)) + $(call DELFILE, $(MICROWINDOWS_TARBALL)) +endif + +CFLAGS += -DRTEMS=0 -DPSP=0 -D__ECOS=0 -D__MINGW32__=0 +CFLAGS += -DELKS=0 -D_MINIX=0 -DMSDOS=0 -DLINUX=0 +CFLAGS += -DMACOSX=0 -DTRIMEDIA=0 Review Comment: They should not pollute global NuttX CFLAGS. So they can be and are defined in local `Makefile` and are used only in private parts of the Microwindows. If they are used in some public header file file then they would be required even for applications because Microwindows use only `#if MSDOS` etc. not `#ifdef` and undefined macros would lead to warnings. It has not been problem in the past but `-Wundef` is defined in NuttX `Toolchain.defs` and may be it is even enabled in modern GCC by default by `-Wall`, etc. Generally, I see as the best solution do not need for these to be defined by build system to zero. One option is to protects `#if` parts by `#if defined(MSDOS) && MSDOS` which prevents (tested on GCC 14.2.0) ``` warning: "MSDOS" is not defined, evaluates to 0 [-Wundef] ``` This needs to be done in whole Microwindows and care has to be taken to not introduce some flip in some logic. Another option is to put these zeroing defines in `microwindows/src/include/mwconfig.nuttx` and all other specific includes. When I look at the possible usecases of Microwindows header files, I have noticed that [microwindows/src/include/mwtypes.h](https://github.com/ghaerr/microwindows/blob/master/src/include/mwtypes.h) uses some config defines, i.e. `MWPIXEL_FORMAT` but seems to not include `mwconfig.h` directly or indirectly. There is one exception ``` #if __ECOS #include <ecosmwconfig.h> /*include the eCos configuration "translation" header */ #endif ``` This meas that if this header file is included without preceding `mwconfig.h` include, then there could be a problem. May it be, it has purpose to not include whole `mwconfig.h` there but including defines part in addition to or instead of `__ECOS` could be there ``` #ifdef MWCONFIG_FILE #define INCFILE(name) name #include INCFILE(MWCONFIG_FILE) #endif ``` On the other hand target specific include fragments do not have protection against multiple inclusion and redefines. So I have plea to @ghaerr to discuss which direction he suggest. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
