Fix broken "Releases" Makefile #13787

truboxl · 2021-03-29T17:03:32Z

It's safe to assume all the Makefiles in https://github.com/emscripten-core/emscripten/releases are broken
You can test this by downloading the tarball or zip from there, extract it, cd into it and run make.

I found these problems when trying to package emscripten for Termux / Android. See termux/termux-packages#5043. Should be done prototyping soon...

truboxl · 2021-03-29T18:06:40Z

Very funny CircleCI... Failing at the last, most irrelevant test :(

truboxl · 2021-03-30T03:33:42Z

Finally CI works! Forgot to add the errors I am trying to solve...

Problem 1: Extract from releases tarball and run make

~/test $ tar xf 2.0.16.tar.gz
~/test $ cd emscripten-2.0.16/
~/test/emscripten-2.0.16 $ make
./tools/install.py ../emscripten-2.0.16
make: ./tools/install.py: No such file or directory
make: *** [Makefile:9: install] Error 127
~/test/emscripten-2.0.16 $ ls
ls: reading directory '.': No such file or directory

Problem 2: .git does not exist

~/test $ cd emscripten-2.0.16
~/test/emscripten-2.0.16 $ make DESTDIR=../emscripten-2.0.16-alt
./tools/install.py ../emscripten-2.0.16-alt
fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
Traceback (most recent call last):
  File "/data/data/com.termux/files/home/test/emscripten-2.0.16/./tools/install.py", line 103, in <module>
      sys.exit(main())
  File "/data/data/com.termux/files/home/test/emscripten-2.0.16/./tools/install.py", line 98, in main
      add_revision_file(target)
  File "/data/data/com.termux/files/home/test/emscripten-2.0.16/./tools/install.py", line 41, in add_revision_file
      git_hash = subprocess.check_output(['git', 'rev-parse', 'HEAD'], encoding='utf-8').strip()
  File "/data/data/com.termux/files/usr/lib/python3.9/subprocess.py", line 424, in check_output
      return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/data/data/com.termux/files/usr/lib/python3.9/subprocess.py", line 528, in run
      raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['git', 'rev-parse', 'HEAD']' returned non-zero exit status 128.
make: *** [Makefile:9: install] Error 1

truboxl · 2021-04-01T02:35:19Z

@kripken @sbc100
Any comments?

sbc100 · 2021-04-05T01:33:01Z

I think when I wrote this this Makefile and install.py script it imagined it as a way to generate the source archive.. not as something that could be run from within the source archive (hence the hard dependency on the git directory).

Perhaps we should remove install.py from the source archive? Can you instead just copy the entire archive contents?

sbc100 · 2021-04-05T01:28:20Z

Makefile

@@ -1,5 +1,7 @@
 VERSION = $(shell cat emscripten-version.txt | sed s/\"//g)
-DESTDIR ?= ../emscripten-$(VERSION)
+ifeq ($(DESTDIR),)
+	DESTDIR := $(shell mktemp -d)/emscripten-$(VERSION)


Isn't the ?= syntax good enough here? Do we want to support DESTDIR being empty?

Actually, both can handle empty DESTDIR...
The problem is that ?= will have different values when DESTDIR is needed, whereas := is defined once. This is problematic for mktemp -d.
I can't find a one line equivalent of ?= that handles both empty and defined once...

EDIT: looks like this isn't compatible with BSD make, I will think of something else...

truboxl · 2021-04-05T07:07:40Z

I think when I wrote this this Makefile and install.py script it imagined it as a way to generate the source archive.. not as something that could be run from within the source archive (hence the hard dependency on the git directory).

Perhaps we should remove install.py from the source archive? Can you instead just copy the entire archive contents?

I rely on install.py clean up functionality. It saved quite a lot of spaces. Very beneficial for downstream packaging...

Using "../emscripten-$(VERSION)" for "DESTDIR" has various problems Notably it self destructs the source as the extracted sources has the same name as "DESTDIR" Fix it by making a unique temporary directory and point there

This is true for all "Releases" extracted sources

sbc100 · 2021-04-05T13:35:51Z

I think when I wrote this this Makefile and install.py script it imagined it as a way to generate the source archive.. not as something that could be run from within the source archive (hence the hard dependency on the git directory).
Perhaps we should remove install.py from the source archive? Can you instead just copy the entire archive contents?

I rely on install.py clean up functionality. It saved quite a lot of spaces. Very beneficial for downstream packaging...

But shouldn't the source archive you down already be the result of "install.py"? Oh no I see now that it is the result of make dist which does not run install.py.

sbc100 · 2021-04-05T13:37:45Z

Makefile

@@ -1,5 +1,6 @@
 VERSION = $(shell cat emscripten-version.txt | sed s/\"//g)
-DESTDIR ?= ../emscripten-$(VERSION)
+TMPDESTDIR := $(shell mktemp -d)
+DESTDIR ?= $(TMPDESTDIR)/emscripten-$(VERSION)


I still don't understand that problem this is solving. Can you explain?

../emscripten-$(VERSION) is exactly the same as $PWD when you first extracted from zip

It will end up delete itself down the line...

EDIT: sorry got confused a bit, err, I think you need to run make just to see what I am getting into

I see, but as long as you run with make DESTDIR=xxx it should be fine right?

Should we just error out if $DESTDIR already exists instead of deleting it?

I feel that running just make and errors out is wrong, lol
and tools/install.py has that error implemented so it must point to folder that is clean

Actually now I am not using this Makefile directly anymore as I opted to use the patched install.py. I just want to raise this issue whether it make sense to still use this Makefile from the "Releases" zips...

Ok I suppose that's the way I guess
The archlinux guys seem to package using make install DESTDIR=xxx and from the git repo
https://github.com/archlinux/svntogit-community/blob/packages/emscripten/trunk/PKGBUILD

I think I will try to change from zip archive to git repo source somehow...

truboxl · 2021-04-05T15:39:48Z

@sbc100
I think I will close this if you are okay with it. I think I can get away from not using "Releases" zips and use git clone now.
So that should solve most of the issues for me...

And because I am actually "cross compiling", the Makefile is also not good as it does a host npm install.
I guess I have to invoke install.py directly.

truboxl · 2021-04-10T09:02:48Z

I am closing this. Can you please check out termux/termux-packages#6578? Thanks.

truboxl closed this Mar 30, 2021

truboxl reopened this Mar 30, 2021

truboxl force-pushed the patch-1 branch from 9249d42 to f50551c Compare March 30, 2021 02:05

sbc100 reviewed Apr 5, 2021

View reviewed changes

truboxl added 2 commits April 5, 2021 16:15

Use unique temporary directory for default DESTDIR

022b9a5

Using "../emscripten-$(VERSION)" for "DESTDIR" has various problems Notably it self destructs the source as the extracted sources has the same name as "DESTDIR" Fix it by making a unique temporary directory and point there

Skip add_revision_file if ".git" not found

d78cc90

This is true for all "Releases" extracted sources

truboxl force-pushed the patch-1 branch from f50551c to d78cc90 Compare April 5, 2021 08:18

sbc100 reviewed Apr 5, 2021

View reviewed changes

truboxl closed this Apr 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix broken "Releases" Makefile #13787

Fix broken "Releases" Makefile #13787

truboxl commented Mar 29, 2021 •

edited

Loading

truboxl commented Mar 29, 2021

truboxl commented Mar 30, 2021

truboxl commented Apr 1, 2021

sbc100 commented Apr 5, 2021

sbc100 Apr 5, 2021

truboxl Apr 5, 2021 •

edited

Loading

truboxl commented Apr 5, 2021

sbc100 commented Apr 5, 2021

sbc100 Apr 5, 2021

truboxl Apr 5, 2021 •

edited

Loading

sbc100 Apr 5, 2021

truboxl Apr 5, 2021

truboxl Apr 5, 2021 •

edited

Loading

truboxl Apr 5, 2021

truboxl commented Apr 5, 2021

truboxl commented Apr 10, 2021

Fix broken "Releases" Makefile #13787

Fix broken "Releases" Makefile #13787

Conversation

truboxl commented Mar 29, 2021 • edited Loading

truboxl commented Mar 29, 2021

truboxl commented Mar 30, 2021

truboxl commented Apr 1, 2021

sbc100 commented Apr 5, 2021

sbc100 Apr 5, 2021

Choose a reason for hiding this comment

truboxl Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

truboxl commented Apr 5, 2021

sbc100 commented Apr 5, 2021

sbc100 Apr 5, 2021

Choose a reason for hiding this comment

truboxl Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

sbc100 Apr 5, 2021

Choose a reason for hiding this comment

truboxl Apr 5, 2021

Choose a reason for hiding this comment

truboxl Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

truboxl Apr 5, 2021

Choose a reason for hiding this comment

truboxl commented Apr 5, 2021

truboxl commented Apr 10, 2021

truboxl commented Mar 29, 2021 •

edited

Loading

truboxl Apr 5, 2021 •

edited

Loading

truboxl Apr 5, 2021 •

edited

Loading

truboxl Apr 5, 2021 •

edited

Loading