Made Makefile cross-platform (Linux, Mac) #97

daveyproctor · 2017-07-17T01:30:52Z

Indeed, thanks to @crossroads1112 for step by step guidance and @americanhanko for his first pass in PR 86

cmlsharp · 2017-07-17T05:13:17Z

I looked this over with Davey before he submitted the PR. Looks good to me!

Only thing I might note in the README.md is that sudo make install is only required on Linux, not on OSX.

kzidane · 2017-07-17T16:22:36Z

Makefile

 	cp -r build/* $(DESTDIR)
-	cp -r debian/docs/* $(DESTDIR)/share/man/man3
+	cp debian/docs/* $(DESTDIR)/$(MANDIR)


@crossroads1112 @daveyproctor is ldconfig needed for Mac?

@kzidane are you asking about the command export LD_LIBRARY_PATH=/usr/local/lib asked for on Linux per line 35 of the updated README.md?

As per Apple Dynamic Libraries documentation, /usr/local is automatically checked for libraries.

A user can install elsewhere as desired, setting LD_LIBRARY_PATH.

Sounds good! Mind running ldconfig if installing on Linux, please?

americanhanko · 2017-07-17T17:20:22Z

This permissions issue is still occuring for me on macOS. This wouldn't happen if there was a formula added for Homebrew installs, but not sure if you want this working without that or not.

$ make install
cc -Wall -Wextra -Werror -pedantic -std=c99 -fPIC -shared -Wl,-install_name,libcs50-8.dylib -o libcs50-8.0.6.dylib src/cs50.c
ln -s libcs50-8.0.6.dylib libcs50-8.dylib
ln -s libcs50-8.dylib libcs50.dylib
mkdir -p src build/include
install -m 644 src/cs50.h build/include/cs50.h
mkdir -p build/lib build/src/libcs50
mv libcs50-8.0.6.dylib libcs50-8.dylib libcs50.dylib build/lib
cp -r src/cs50.c src/cs50.h build/src/libcs50
asciidoctor -d manpage -b manpage -D debian/docs/ docs/eprintf.adoc docs/get_char.adoc docs/get_double.adoc docs/get_float.adoc docs/get_int.adoc docs/get_long_long.adoc docs/get_string.adoc
mkdir -p /usr/local /usr/local/share/man/man3
cp -r build/* /usr/local
cp: /usr/local/src: Permission denied
cp: build/src: unable to copy extended attributes to /usr/local/src: Permission denied
cp: /usr/local/src/libcs50: No such file or directory
cp: build/src/libcs50: unable to copy extended attributes to /usr/local/src/libcs50: No such file or directory
cp: /usr/local/src/libcs50/cs50.c: No such file or directory
cp: /usr/local/src/libcs50/cs50.h: No such file or directory
make: *** [install] Error 1```

daveyproctor · 2017-07-18T14:02:57Z

@americanhanko mind showing me ls -l /usr/local/src?
I'm also wondering if sudo make install works?

daveyproctor · 2017-07-18T14:33:51Z

@kzidane ideas on how to pass travis-ci tests?

kzidane · 2017-07-18T14:38:33Z

@daveyproctor don't worry about that! It's complaining because you're PR'ing from a fork, and we have some environment variables set that are not available when building PRs from forks for security reasons. If you have write-access to this repo, you may push a branch and PR from that. Otherwise, I can fix later after merging, instead of having you worry about modifying the .travis.yml file. How does that sound?

cmlsharp · 2017-07-20T18:58:51Z

daveyproctor · 2017-07-21T13:14:26Z

@dmalan ready to merge?

kzidane · 2017-08-21T15:23:15Z

@dmalan any takes on this one?

americanhanko · 2017-08-22T18:24:26Z

@daveyproctor Apologies for the delay in my response! sudo make install works on macOS and files that include cs50.h compile successfully with both gcc and clang. As expected, it does set root as the owner of/usr/local/src/. I don't foresee any issues with that though unless something else were to utilize that directory with different permissions.

Given all that, I do think a more elegant solution to installation on macOS would be to create a Homebrew recipe for libcs50 that utilizes the changes in this PR. This way, a student using macOS could install, uninstall, update, etc. with brew. Any thoughts on that?

Either way, thanks! Great work on this.

daveyproctor · 2017-08-22T19:19:57Z

@americanhanko I did a little research on this, per the below. Unless I'm missing something(?), a problem with Brew seems to be that even upping the version number requires a PR. Rather unergonomic, if so. My full review:

Getting libcs50 via Brew

Goal

$ brew install libcs50
==> Using the sandbox
==> Downloading https://github.com/cs50/libcs50/archive/v8.0.5.zip
######################################################################## 100.0%
==> make install
🍺  /usr/local/Cellar/libcs50/8.0.5: 14 files, 33.2KB, built in 2 seconds

Steps and UX

Create a brew formula how to: Formula Cookbook
- Everything happens via Github.
- Fork the homebrew-core repo, add a Ruby "formula", and PR.
- example formulas
- $brew create https://github.com/cs50/libcs50/archive/v8.0.5.zip opens a pre-populated file /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/libcs50.rb
- Formula contains
  - a link to source zip / tarball
  - Minimum 1 test to make sure library is installed properly
On brew install cs50, Brew
- gets and extracts the library
- runs ./configure and make install, by default
  - installed into /usr/local/Cellar/
  - symlinks /usr/local/* to the Cellar
- runs tests specified in the Formula
On brew upgrade cs50, Brew
- checks the current version against the formula in GitHub
- reinstalls as needed

Concerns + workarounds

As they say in the Formula Cookbook, upping the library version involves creating a new pull request with a trivially-changed brew formula.
- Example.
- This is bad because we would be reliant on brew in order to keep our libary versions in sync via multiple download methods.
- Workaround
  - Brew makes this somewhat ergonomic with the command brew bump-formula-pr, which forks, commits, and pushes all in one step. Link
    - Also, brew bump-formula-pr --help
Installing into /usr/local/Cellar and symlinking /usr/local/* to it is different from installing from source on Mac and Linux, as per PR 97
- Workaround
  - We could keep a different Makefile, Makefile.brew for Brew to use. In our formula, we could do make -f Makefile.brew

americanhanko · 2017-08-22T19:37:54Z

@daveyproctor thanks for the quick reply (and the excellent review).

We could keep a different Makefile, Makefile.brew for Brew to use. In our formula, we could do make -f Makefile.brew

This is one of the main reasons I chose to create Makefile.macos in #86. If keeping a different Makefile for Homebrew is legitimately an option, feel free to take from the one I wrote. Also, I completely agree with naming it Makefile.brew - it would better communicate its purpose.

Perhaps another option would be to write another target definition in the Makefile (e.g. make brew) which could be called in the Homebrew formula.

cmlsharp · 2017-08-23T15:51:14Z

Homebrew concerns aside, I think we should merge this PR so we get rudimentary support for OSX and support for locally-installed asciidoctor (a nice side-effect of build and docs no longer being PHONY). We can worry about Homebrew after that.

daveyproctor · 2017-08-27T00:39:18Z

@crossroads1112 would you consider pre-building the man pages on our end so the end user doesn't need to worry about asciidoctor?

cmlsharp · 2017-08-27T00:46:35Z

So every time a change is made to the adoc files, we'd have to remember to regenerate the man pages? That doesn't feel super clean to me but maybe there's a better way? In any case, I don't really have a problem with keeping asciidoctor as a dependency.

daveyproctor · 2017-08-27T01:46:24Z

That CS50 generates the man pages from .adoc seems like a convenience for us more than being relevant to the user. For instance, a maker of a library could write straight to their raw man pages, right? It almost seems like the .adoc files should live in our .gitignore, and not come with the library at all (only the man pages). Do you happen to know if asciidoctor or other man page generator is often a dependency for library installs?

cmlsharp · 2017-08-27T11:23:31Z

It is absolutely a convenience for us but I don't see why that a bad thing. I don't know about asciidoctor specifically but it is very common for software to have "build dependencies" which are only required to make the project in the first place and are thus inherently conveniences for the developers as they aren't relavent to the actual use of the software.

americanhanko · 2017-08-27T23:46:40Z

@crossroads1112

So every time a change is made to the adoc files, we'd have to remember to regenerate the man pages?

Don't you have to do that anyway since the Debian package is distributed with the compiled man pages?

cmlsharp · 2017-08-28T00:55:59Z

It happens every time you run `make deb` I think. In any case, this is really outside the scope of this PR

…

On Aug 27, 2017 7:46 PM, "Eric Hanko" ***@***.***> wrote: @crossroads1112 <https://github.com/crossroads1112> So every time a change is made to the adoc files, we'd have to remember to regenerate the man pages? Don't you have to do that anyway since the Debian package is distributed with the compiled man pages? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#97 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADfDr8KJK7midSyWhTlUI-3wUoHzq1r-ks5scf_ggaJpZM4OZesQ> .

cmlsharp · 2017-08-28T01:09:05Z

To clarify my previous comment, it feels very unclean to maintain two different versions of the man pages that, in between releases, are not necessarily in sync. Avoiding the `asciidoctor` dependency for the `build` target doesn't feel like a compelling enough reason to do this, particularly since only more comfy students will be downloading the library on there own anyway. I think this issue merits discussion but should be done in a separate issue/PR. I think this PR should be merged in the meantime while we figure this (and Homebrew) out. Thoughts @dmalan?

…

On Aug 27, 2017 8:55 PM, "Chad Sharp" ***@***.***> wrote: It happens every time you run `make deb` I think. In any case, this is really outside the scope of this PR On Aug 27, 2017 7:46 PM, "Eric Hanko" ***@***.***> wrote: > @crossroads1112 <https://github.com/crossroads1112> > > So every time a change is made to the adoc files, we'd have to remember > to regenerate the man pages? > > Don't you have to do that anyway since the Debian package is distributed > with the compiled man pages? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#97 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ADfDr8KJK7midSyWhTlUI-3wUoHzq1r-ks5scf_ggaJpZM4OZesQ> > . >

kzidane · 2017-08-29T07:09:30Z

Merged as part of #106. Thank you!

dmalan requested review from cmlsharp and kzidane July 17, 2017 01:46

kzidane reviewed Jul 17, 2017

View reviewed changes

daveyproctor force-pushed the develop branch from 0a04d2c to 645d8dd Compare July 18, 2017 18:35

cmlsharp mentioned this pull request Jul 18, 2017

Ressurect the /usr vs /usr/local discussion #79

Closed

daveyproctor force-pushed the develop branch 2 times, most recently from a8dc460 to eaecbd7 Compare July 20, 2017 18:48

cmlsharp previously approved these changes Jul 20, 2017

View reviewed changes

Made Makefile cross-platform (Linux, Mac)

16ebff9

daveyproctor dismissed cmlsharp’s stale review via 16ebff9 July 20, 2017 21:10

daveyproctor force-pushed the develop branch from eaecbd7 to 16ebff9 Compare July 20, 2017 21:10

cmlsharp previously approved these changes Jul 21, 2017

View reviewed changes

kzidane mentioned this pull request Jul 25, 2017

dists/signing #104

Closed

updated readme with troubleshooting info

49ffba1

daveyproctor dismissed cmlsharp’s stale review via 49ffba1 July 28, 2017 16:08

cmlsharp approved these changes Aug 21, 2017

View reviewed changes

cmlsharp mentioned this pull request Aug 22, 2017

Mac Installation #105

Closed

kzidane mentioned this pull request Aug 22, 2017

Makefile.macos and Homebrew compatibility #86

Closed

cmlsharp mentioned this pull request Aug 27, 2017

Outdated installation instructions in CS50 Manual #57

Closed

kzidane mentioned this pull request Aug 29, 2017

Makefile/dists/signing #106

Merged

kzidane closed this Aug 29, 2017

americanhanko mentioned this pull request Sep 3, 2017

README does not indicate that sudo is needed for macOS #108

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made Makefile cross-platform (Linux, Mac) #97

Made Makefile cross-platform (Linux, Mac) #97

daveyproctor commented Jul 17, 2017 •

edited

Loading

cmlsharp commented Jul 17, 2017 •

edited

Loading

kzidane Jul 17, 2017

daveyproctor Jul 18, 2017 •

edited

Loading

kzidane Jul 18, 2017

americanhanko commented Jul 17, 2017 •

edited

Loading

daveyproctor commented Jul 18, 2017 •

edited

Loading

daveyproctor commented Jul 18, 2017

kzidane commented Jul 18, 2017

cmlsharp commented Jul 20, 2017

daveyproctor commented Jul 21, 2017

kzidane commented Aug 21, 2017

americanhanko commented Aug 22, 2017 •

edited

Loading

daveyproctor commented Aug 22, 2017

americanhanko commented Aug 22, 2017

cmlsharp commented Aug 23, 2017 •

edited

Loading

daveyproctor commented Aug 27, 2017

cmlsharp commented Aug 27, 2017

daveyproctor commented Aug 27, 2017

cmlsharp commented Aug 27, 2017

americanhanko commented Aug 27, 2017

cmlsharp commented Aug 28, 2017 via email

cmlsharp commented Aug 28, 2017 via email

kzidane commented Aug 29, 2017

Made Makefile cross-platform (Linux, Mac) #97

Made Makefile cross-platform (Linux, Mac) #97

Conversation

daveyproctor commented Jul 17, 2017 • edited Loading

cmlsharp commented Jul 17, 2017 • edited Loading

kzidane Jul 17, 2017

Choose a reason for hiding this comment

daveyproctor Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

kzidane Jul 18, 2017

Choose a reason for hiding this comment

americanhanko commented Jul 17, 2017 • edited Loading

daveyproctor commented Jul 18, 2017 • edited Loading

daveyproctor commented Jul 18, 2017

kzidane commented Jul 18, 2017

cmlsharp commented Jul 20, 2017

daveyproctor commented Jul 21, 2017

kzidane commented Aug 21, 2017

americanhanko commented Aug 22, 2017 • edited Loading

daveyproctor commented Aug 22, 2017

Getting libcs50 via Brew

Goal

Steps and UX

Concerns + workarounds

americanhanko commented Aug 22, 2017

cmlsharp commented Aug 23, 2017 • edited Loading

daveyproctor commented Aug 27, 2017

cmlsharp commented Aug 27, 2017

daveyproctor commented Aug 27, 2017

cmlsharp commented Aug 27, 2017

americanhanko commented Aug 27, 2017

cmlsharp commented Aug 28, 2017 via email

cmlsharp commented Aug 28, 2017 via email

kzidane commented Aug 29, 2017

daveyproctor commented Jul 17, 2017 •

edited

Loading

cmlsharp commented Jul 17, 2017 •

edited

Loading

daveyproctor Jul 18, 2017 •

edited

Loading

americanhanko commented Jul 17, 2017 •

edited

Loading

daveyproctor commented Jul 18, 2017 •

edited

Loading

americanhanko commented Aug 22, 2017 •

edited

Loading

cmlsharp commented Aug 23, 2017 •

edited

Loading