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

plist.mk: Cheap fix for PLIST madness found with Issue #4211 #4221

Closed
wants to merge 1 commit into from

Conversation

th0ma7
Copy link
Contributor

@th0ma7 th0ma7 commented Oct 15, 2020

Motivation: Fix PLIST black-magic issue found
Linked issues: #4211

Checklist

  • Build rule all-supported completed successfully
  • Package upgrade completed successfully
  • New installation of package completed successfully

@th0ma7
Copy link
Contributor Author

th0ma7 commented Oct 15, 2020

@hgy59 and @ymartin59 and all, here is a temporary fix proposal for the PLIST issue as documented in #4211.
Preliminary test shows that is works against fossil-scm.
Requires more testing...

@th0ma7
Copy link
Contributor Author

th0ma7 commented Oct 15, 2020

Solution ain't sufficient, needs additional thinking or complete rewrite of the PLIST code.
This quick fix proposal doesn't handle per arch sub-dependencies which are then considered mandatory for all arches.
Complex package such as ffmpeg is a good one to test with.

@@ -9,14 +9,13 @@ endif

.PHONY: cat_PLIST
cat_PLIST:
@for depend in $(DEPENDS) ; \
@echo Processing PLIST dependencies 1>&2
@for depend in `$(MAKE) dependency-list` ; \
Copy link
Contributor

@hgy59 hgy59 Oct 16, 2020

Choose a reason for hiding this comment

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

If you take dependency-list instead of $(DEPENDS) you get the dependencies of $(BUILD_DEPENDS) $(DEPENDS) $(OPTIONAL_DEPENDS)
but this will fail because all files in PLIST must exist (and the optional are dependencies that exist not for all archs).
And BUILD_DEPENDS is for packages that are used only in the build environment and not needed at runtime (i.e. dependency of another spk like python).
If dependency-list fixes the PLIST file for some packages, those packages may have used BUILD_DEPENDS but should have used DEPENDS.
As for fossil-scm if the error you meant in #4222 is missing PLIST files of openssl or zlib, then the real error is that fossil-scm has defined BUILD_DEPENDS instead of DEPENDS for cross/zlib and cross/openssl.

@hgy59
Copy link
Contributor

hgy59 commented Oct 16, 2020

Just used grep and found false usage of BUILD_DEPENDS in

  • cross/autossh
  • cross/he853
  • cross/tcl
  • spk/borgbackup
  • spk/duplicity
  • spk/ffsync
  • spk/mercurial

@th0ma7
Copy link
Contributor Author

th0ma7 commented Oct 17, 2020

Just used grep and found false usage of BUILD_DEPENDS in

How do you do to you determine if theses are false usages of BUILD_DEPENDS ?

@hgy59
Copy link
Contributor

hgy59 commented Oct 17, 2020

Just used grep and found false usage of BUILD_DEPENDS in

How do you do to you determine if theses are false usages of BUILD_DEPENDS ?

if a package has BUILD_DEPENDS on it's own it must fail, as it is not (properly) packaged into the spk.

  • may be I am wrong here (cross/duplicity and cross/borgbackup are wheels)

cross/tcl:

  • this package links to zlib (I cannot see any options that zlib is statically linked), and must define DEPENDS = cross/zlib

cross/autossh:

  • this package links to openssh and must define DEPENDS = cross/openssh

cross/he853:

  • this package links to cross/libusb and must define DEPENDS = cross/libusb

spk/ffsync:

  • here I am not sure, as cross/gevent cross/greenlet may be used for building wheels only. But I guess these two libraries are needed at runtime and not only at build time.

spk/mercurial:

  • another wheel package, where I don't know whether the wheel includes all the dependencies or not.

So my statement was hasty - not regarding the dependencies of wheels - but for non python wheels following packages must use DEPENDS instead of BUILD_DEPENDS:

  • cross/autossh
  • cross/he853
  • cross/tcl

and

  • cross/fossil-scm

@ymartin59
Copy link
Contributor

I am curious to know what "code changes" has introduced bug to better understand how to get it fixed. Does it come from moving targets to dedicated "plist.mk" file?

@th0ma7
Copy link
Contributor Author

th0ma7 commented Oct 21, 2020

I am curious to know what "code changes" has introduced bug to better understand how to get it fixed. Does it come from moving targets to dedicated "plist.mk" file?

There was no code change from the framework point of view. Also the changes I recently did had no effect on the PLIST part of the framework.

If there was a change it may end-up being in a dependency being build only with shared libs or a change in configuration option that make a binary to not link against the expected static lib but rather needing the shared lib instead.

Thus, from @hgy59 analysis and comment above, some of the packages must use DEPENDS instead of BUILD_DEPENDS. No one yet took action AFAIK on that. Part of the proposed list figures also fossil-scm, package which me starting this whole thread originating from #4211 (comment) and proposed approach in this "tentative" PR.

Lastly, note that code in this PR is irrelevant (thnx to @hgy59)... although discussions may lead at finding a better approach.

@hgy59
Copy link
Contributor

hgy59 commented Oct 29, 2020

I am curious to know what "code changes" has introduced bug to better understand how to get it fixed. Does it come from moving targets to dedicated "plist.mk" file?

The code change for fossil was with commit 26ad900 on cross/fossil-scm/Makefile
26ad900#diff-8a946aa031b07b52cf3ae47688cfe3536e3dba5ad14d011e6fa45c4c0c4659e4

by removing --static from CONFIGURE_ARGS without changing BUILD_DEPENDS to DEPENDS.

@hgy59 hgy59 mentioned this pull request Oct 29, 2020
3 tasks
@ymartin59
Copy link
Contributor

As a local mistake in fossil-scm I propose to close without merge

@th0ma7
Copy link
Contributor Author

th0ma7 commented Oct 31, 2020

Agreed.
@hgy59 Before closing, is there any action left to do considering your previous comment #4221 (comment) ? Should another issue be opened to track it?

@hgy59 hgy59 mentioned this pull request Nov 5, 2020
3 tasks
@hgy59
Copy link
Contributor

hgy59 commented Nov 5, 2020

Agreed.
@hgy59 Before closing, is there any action left to do considering your previous comment #4221 (comment) ? Should another issue be opened to track it?

@th0ma7 PR #4257 created

@hgy59 hgy59 closed this Nov 5, 2020
@th0ma7 th0ma7 deleted the cheap-plist-fix branch April 23, 2021 14:30
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.

3 participants