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

class.sources that are added in a forLinux clause are not built #61

Closed
umlaeute opened this issue Mar 3, 2020 · 4 comments
Closed

class.sources that are added in a forLinux clause are not built #61

umlaeute opened this issue Mar 3, 2020 · 4 comments

Comments

@umlaeute
Copy link
Contributor

umlaeute commented Mar 3, 2020

i just stumbled upon this while modernizing hcs.

if i have an objectclass that should only build for a given platform, the documentation says to wrap it into a define forXXX clause:

# forLinux, forDarwin, forWindows:
# Shorthand for 'variable definitions for Linux only' etc. Use like:
# define forLinux
# cflags += -DLINUX
# class.sources += linuxthing.c
# endef

However, doing so does not actually work: the specified classes are not build at all.
However, the define does work, as can be seen from other variables used in the clause.

consider the following replacement of the helloworld Makefile:

# Makefile to build class 'helloworld' for Pure Data.
# Needs Makefile.pdlibbuilder as helper makefile for platform-dependent build
# settings and rules.

# library name
lib.name = helloworld

# input source file (class name == source file basename)
class.sources =
datafiles = helloworld-meta.pd README.md

define forLinux
  class.sources += test.c
  datafiles += helloworld-help.pd
endef

# include Makefile.pdlibbuilder from submodule directory 'pd-lib-builder'
PDLIBBUILDER_DIR=pd-lib-builder/
include $(PDLIBBUILDER_DIR)/Makefile.pdlibbuilder

Running make does not do anything.
Running make install will install the following files:

$ ls -1 .../helloworld/
helloworld-help.pd
helloworld-meta.pd
README.md

so the "helloworld.pd_linux" is missing, but the "helloworld-help.pd" gets installed.

@umlaeute
Copy link
Contributor Author

umlaeute commented Mar 3, 2020

it seems this never worked for any released version of pd-lib-builder.

i'm testing like this:

(cd pd-lib-builder; git tag | egrep "^v") | while read tag
do
    rm *.o *_*
    (cd pd-lib-builder/; git checkout ${tag})
    make && make install DESTDIR=$(pwd)/${tag}
    rm *.o *_*
done

find v*/ -type f  -name "*helloworld*" -not -name "*.pd"

@katjav
Copy link
Contributor

katjav commented Mar 3, 2020

This can't work indeed. The define forXXX method was added in response to issue #7 (Enable setting flags for OS). It works for flags but the example class.sources += linuxthing.c must have been made up without testing. Running make vars shows that cumulative variables like classes.sources and classes.objects are empty. The reason is because these variables are expanded before the platform detection section.

Now I tried swapping these sections and in that case the cumulative variables for file names are expanded correctly, and it does build. Actually I like this order of sections better; platform detection first, then build flags accumulation, then file name accumulation. Also the platform-dependent name definition of shared.lib as discusssed in #59 would get a more logical place. But variable reshuffling in a makefile is hazardous and can easily give regressions, so I'd like to give this some more thought and testing.

@umlaeute
Copy link
Contributor Author

umlaeute commented Mar 4, 2020

But variable reshuffling in a makefile is hazardous and can easily give regressions

which brings us to #44 :-)

katjav added a commit that referenced this issue Mar 8, 2020
This solves the issue where platform-dependent class inclusion
could not work (#61). Also it seems a more logical order of
evaluation in general because binary file extensions are platform-
dependent.
@katjav
Copy link
Contributor

katjav commented Mar 8, 2020

Fixed as discussed above with commit a6975e9.

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

No branches or pull requests

2 participants