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

Fix ndless_installer_4.5 on macOS #200

Merged
merged 2 commits into from
Oct 25, 2020
Merged

Fix ndless_installer_4.5 on macOS #200

merged 2 commits into from
Oct 25, 2020

Conversation

unilock
Copy link
Contributor

@unilock unilock commented Feb 20, 2020

Mostly untested. Assumes the following have been installed with brew:

  • binutils
  • boost
  • gcc
  • git
  • gmp
  • libmpc
  • mpfr
  • wget
  • zlib

May break if more than one gcc version is installed.

@adriweb
Copy link
Contributor

adriweb commented Feb 20, 2020

I'll try that later as I need to update my toolchain anyway and I previously made some changes about that...
I'm not sure everything in the patch is actually needed. I built the toolchain just fine with clang, which allows to avoid all the gcc version stuff altogether since it'll take the default xcode one.
I believe the only change I previously needed to make was to remove with-python from the GDB build options. Which, funnily enough, isn't part of this patch.

payload: ndless_installer.bin
(../tools/MakeChunkDispatch/MakeChunkDispatch 0x1800e7f4 0x12480000; cat $^) | base64 -w0 > $@ || rm $@
ifeq ($(OS_NAME),darwin)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be done by using feature detection instead. base64 doesn't have/need the -w0 option on some Linux distros either.

@Vogtinator
Copy link
Contributor

I don't think the brew specific changes make sense. AFAIK brew (optionally?) sets up $PATH, so it should already find everything correctly.

The unalias calls in ./bulid_toolchain.sh are noops anyway as they only influence the current shell, which is exited directly afterwards.

@adriweb
Copy link
Contributor

adriweb commented Feb 20, 2020

So, I've been able to use build_toolchain.sh just fine on macOS today, I just updated the versions of the deps: #201
I also removed --with-python because I had problems in the past with it.

So I guess your modifications aren't actually needed? Must be some configuration on your side, for brew etc. I don't believe I have anything special in my PATH to make it work...

Regarding the base64 thing in installer-4.5, yeah, we need to have a solution properly, as it wouldn't work on macOS otherwise.

Anyway, I agree with what @Vogtinator said, otherwise.

@unilock
Copy link
Contributor Author

unilock commented Feb 21, 2020

@adriweb That's strange; I ran into errors when first attempting to build the toolchain with clang. Though, trying it again with only the changes from #201 applied, I'm able to build it just fine (notably with --with-python).
So my changes to build_toolchain.sh were unnecessary.

Copy link
Contributor

@Vogtinator Vogtinator left a comment

Choose a reason for hiding this comment

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

Can you drop the changes to build_toolchain.sh and make the installer-4.5/Makefile use feature detection? Then I'll merge.

@unilock unilock changed the title Fix building toolchain + ndless_installer_4.5 on macOS Fix ndless_installer_4.5 on macOS Jun 25, 2020
@unilock
Copy link
Contributor Author

unilock commented Jun 25, 2020

@Vogtinator I believe I've done as you asked; I'm not 100% certain if my modification actually works. Notably, while rebuilding the toolchain on macOS to test my handiwork, I ran into the following issue:

Extracting binutils-2.34.tar.bz2...
tar: Option -a is not permitted in mode -x

Removing -a from tar in build_toolchain.sh seems to have fixed this. Not sure why it was there in the first place.

@adriweb
Copy link
Contributor

adriweb commented Jun 25, 2020 via email

@unilock
Copy link
Contributor Author

unilock commented Jun 25, 2020

ifeq ($(shell base64 -w 2>/dev/null),) is probably more correct than ifeq ($(base64 -w 2>/dev/null),) - but apparently still incorrect, as I run into

ifeq (,)
/bin/sh: -c: line 0: syntax error near unexpected token `,'
/bin/sh: -c: line 0: `ifeq (,)'

when trying to compile my crummy code.
If someone who knows what they're doing wants to implement "feature detection" for the base64 command, by all means, go ahead.

I can at least say with confidence that the toolchain compiles successfully on macOS, once -a is removed from tar.

@Vogtinator
Copy link
Contributor

ifeq ($(shell base64 -w 2>/dev/null),) is probably more correct than ifeq ($(base64 -w 2>/dev/null),)

Yeah, I don't think $(base64 ...) has the desired effect as there's no base64 make builitin.

  • but apparently still incorrect, as I run into

That means it was run as part of the rule command passed to sh, maybe just drop the indentation? See https://www.gnu.org/software/make/manual/html_node/Recipe-Syntax.html#Recipe-Syntax. If that doesn't work, you could do the conditional evaluation as part of the shell recipe (though everything has to be on a single logical line).

I can at least say with confidence that the toolchain compiles successfully on macOS, once -a is removed from tar.

For autodetection of the compression algorithm, but that might not . Just to be sure, you didn't change anything other than -xaf -> -xf, right?

@fridtjof
Copy link
Contributor

fridtjof commented Aug 4, 2020

I can at least say with confidence that the toolchain compiles successfully on macOS, once -a is removed from tar.

For autodetection of the compression algorithm, but that might not . Just to be sure, you didn't change anything other than -xaf -> -xf, right?

Apparently, the tar macOS uses (BSD tar) always auto detects compression formats without any special parameters:

https://www.freebsd.org/cgi/man.cgi?tar(1)

CTRL+F for "compression automatically when reading archives"

I can also confirm removing -a does the trick on macOS.

fridtjof added a commit to shinyblink/Ndless that referenced this pull request Aug 4, 2020
macOS failure is expected at this time, see ndless-nspire#200 (comment)
Vogtinator added a commit that referenced this pull request Aug 5, 2020
Seems like both BSD and GNU tar autodetect the method for decompression,
but BSD tar doesn't like the -a option for extraction.

Fixes part of #200
@Vogtinator
Copy link
Contributor

I can also confirm removing -a does the trick on macOS.

Even better, so both BSD and GNU tar are happy with just -xf. I dropped the -a now.

fridtjof added a commit to shinyblink/Ndless that referenced this pull request Oct 6, 2020
macOS failure is expected at this time, see ndless-nspire#200 (comment)
@Kakise
Copy link

Kakise commented Oct 14, 2020

ifeq ($(shell base64 -w 2>/dev/null),) is probably more correct than ifeq ($(base64 -w 2>/dev/null),)

Yeah, I don't think $(base64 ...) has the desired effect as there's no base64 make builitin.

  • but apparently still incorrect, as I run into

That means it was run as part of the rule command passed to sh, maybe just drop the indentation? See https://www.gnu.org/software/make/manual/html_node/Recipe-Syntax.html#Recipe-Syntax. If that doesn't work, you could do the conditional evaluation as part of the shell recipe (though everything has to be on a single logical line).

I can at least say with confidence that the toolchain compiles successfully on macOS, once -a is removed from tar.

For autodetection of the compression algorithm, but that might not . Just to be sure, you didn't change anything other than -xaf -> -xf, right?

This:

BASE64 := base64 -w0
ifeq ($(shell base64 -w 2> /dev/null),)
    BASE64 := base64
endif

works perfectly on macOS

@N0ury
Copy link

N0ury commented Oct 14, 2020

ifeq ($(shell base64 -w 2>/dev/null),) is probably more correct than ifeq ($(base64 -w 2>/dev/null),)

Yeah, I don't think $(base64 ...) has the desired effect as there's no base64 make builitin.

  • but apparently still incorrect, as I run into

That means it was run as part of the rule command passed to sh, maybe just drop the indentation? See https://www.gnu.org/software/make/manual/html_node/Recipe-Syntax.html#Recipe-Syntax. If that doesn't work, you could do the conditional evaluation as part of the shell recipe (though everything has to be on a single logical line).

I can at least say with confidence that the toolchain compiles successfully on macOS, once -a is removed from tar.

For autodetection of the compression algorithm, but that might not . Just to be sure, you didn't change anything other than -xaf -> -xf, right?

This:

BASE64 := base64 -w0
ifeq ($(shell base64 -w 2> /dev/null),)
    BASE64 := base64
endif

works perfectly on macOS

This is probably not native base64.

base64 -w0
base64: invalid option -- w
Usage:	base64 [-hvDd] [-b num] [-i in_file] [-o out_file]
  -h, --help     display this message
  -Dd, --decode   decodes input
  -b, --break    break encoded string into num character lines
  -i, --input    input file (default: "-" for stdin)
  -o, --output   output file (default: "-" for stdout)

@Kakise
Copy link

Kakise commented Oct 14, 2020

This is probably not native base64.

base64 -w0
base64: invalid option -- w
Usage:	base64 [-hvDd] [-b num] [-i in_file] [-o out_file]
  -h, --help     display this message
  -Dd, --decode   decodes input
  -b, --break    break encoded string into num character lines
  -i, --input    input file (default: "-" for stdin)
  -o, --output   output file (default: "-" for stdout)

Tried with a fresh macOS 10.15.7 install.
Can you try this patch https://gist.github.com/Kakise/637740402370369366cd95e125e64ccb ? It should work

@Vogtinator
Copy link
Contributor

Can you try this patch https://gist.github.com/Kakise/637740402370369366cd95e125e64ccb ? It should work

That would break build on systems where -w0 is necessary, here the condition is also true.

Vogtinator pushed a commit that referenced this pull request Oct 17, 2020
* Add a CI workflow for the toolchain and SDK

* CI: fix toolchain permissions

* CI: use tar to accelerate artifacts

* CI: temporarily force packaging the toolchain, and gz it

* CI: skip packaging again if there's a cache

* CI: fix paths

* CI: fix artifact download path

* CI: minor formatting

* CI: expand SDK build to more OSs

macOS failure is expected at this time, see #200 (comment)

* CI: remove macOS for now

more steps (including a separate toolchain build for macOS) are needed for this

* CI: install necessary dependencies

* CI: separate toolchain for each OS

* CI: add Ubuntu 20.04

* CI: use matrix.os instead of runner.os

* Update .github/workflows/main.yml

use -j4 for make

Co-authored-by: Fabian Vogt <fabian@ritter-vogt.de>

* CI: merge jobs into one

* CI: always install toolchain dependencies

* CI: clarify a step

* CI: remove ubuntu-16.04

* CI: simplify artifact upload

* Revert "CI: simplify artifact upload"

This reverts commit 8c01113.

* ci: remove artifact uploads

* CI: update PATH setup to use environment files

see https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/
@Kakise
Copy link

Kakise commented Oct 19, 2020

Can you try this patch https://gist.github.com/Kakise/637740402370369366cd95e125e64ccb ? It should work

That would break build on systems where -w0 is necessary, here the condition is also true.

How would it break it ? The "BASE64" variable is defaulted to base64 -w0and is changed only if the -w option is not implemented. Or am I missing something ? Genuinely asking because I'm bad with makefiles lol

@Vogtinator
Copy link
Contributor

Can you try this patch https://gist.github.com/Kakise/637740402370369366cd95e125e64ccb ? It should work

That would break build on systems where -w0 is necessary, here the condition is also true.

How would it break it ? The "BASE64" variable is defaulted to base64 -w0and is changed only if the -w option is not implemented. Or am I missing something ? Genuinely asking because I'm bad with makefiles lol

The detection doesn't work. Here the w option is implemented and ifeq ($(shell base64 -w 2> /dev/null),) is still true.

@unilock unilock closed this Oct 20, 2020
@unilock unilock reopened this Oct 20, 2020
…se64 command

Signed-off-by: unilock <unilock@example.com>
@unilock
Copy link
Contributor Author

unilock commented Oct 20, 2020

The latest hastily-pushed commit should work. I didn't have time to test in the context of building Ndless.

@Vogtinator
Copy link
Contributor

The latest hastily-pushed commit should work. I didn't have time to test in the context of building Ndless.

It doesn't seem to work. base64 -w exits with an error here:

> base64 -w
base64: option requires an argument -- 'w'
Try 'base64 --help' for more information.
> echo $?
1

@unilock
Copy link
Contributor Author

unilock commented Oct 24, 2020

It doesn't seem to work. base64 -w exits with an error here:

Ah, that's because -w expects a valid wrap size. Try the new commit.
Only tested on Linux; not macOS. But it "should" work on both.

@Vogtinator
Copy link
Contributor

Only tested on Linux; not macOS. But it "should" work on both.

Yeah, confirmed to work here. The main point of this is to fix it for macOS, so this really should be tested there before merging.

@unilock
Copy link
Contributor Author

unilock commented Oct 25, 2020

Seems like it's working on my end.
Screen Shot 2020-10-25 at 10 32 09 AM

@Vogtinator
Copy link
Contributor

Ok.

The use of $? with pipes can be a bit weird, so could you optimize the pipe away, like base64 -w0 >/dev/null 2>&1 </dev/null?

@unilock
Copy link
Contributor Author

unilock commented Oct 25, 2020

Done. I can confirm that works on both Linux and macOS.

@Vogtinator Vogtinator merged commit 8d9206c into ndless-nspire:master Oct 25, 2020
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.

None yet

6 participants