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

Makefile improvements #10

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

turboencabulator
Copy link
Contributor

Here are several commits intended to make noweb easier to package, since it looks like very few distributions have upgraded to 2.12. Highlights:

  • Improve parallelism and recursion by using $(MAKE) in subshells
  • Add support for $(DESTDIR) and $(PREFIX) variables
  • Use $(CFLAGS) and $(LDFLAGS) when compiling and linking
  • Fix make touch to work as originally intended (to avoid rebuilding generated files)
  • Generate and add all files needed by the awkname script (otherwise can't bootstrap by following the installation instructions)
  • Miscellaneous cleanups and cruft removal

Makefile.gnu and Makefile.make are identical.  Only keep one.
Check their status using && instead of assuming they succeed.

Always execute them in a subshell.  Some flavors of make allow all
commands to run in the same shell.
Remove absolute paths (change /bin/rm, /bin/cp).
Always echo the rm command (change -rm).
Put the clean command on its own line.
On Slackware, it's customary to gzip them as part of creating the
package.  Doing it here isn't necessary.
$< should only be used in suffix rules.
Remove the all-scripts target.
Replace filenames with variables where appropriate.
Install files consistently using cp, not cp -p nor mv.
Don't need to explicitly pass DESTDIR to recursive invocations.
`make touch` was roughly equivalent to `make source` followed by
`make boot`, so remove it and fix up some discrepancies.  Rename the
boot targets to "touch" to restore the original purpose of `make touch`.

New behavior:  source targets build the sources that are distributed in
the tarball; touch and clobber targets will touch and delete those
files, respectively.
Add a PREFIX variable that can be overridden.
Change default LIB directory to /usr/local/libexec/noweb.
Change default MAN directory to /usr/local/share/man.
The FAQ currently can't be regenerated locally, it requires html2ascii,
and also needs the original HTML located at $(HOME)/www/noweb/FAQ.html.
@nrnrnr
Copy link
Owner

nrnrnr commented Feb 12, 2020

Thanks for this! Long overdue.

My PR workflow is embarassingly bad, and I'm teaching a large required course this term. So the PR is likely to sit until June :-(

@siraben
Copy link

siraben commented Feb 28, 2021

Any updates on this?

@nrnrnr
Copy link
Owner

nrnrnr commented Mar 1, 2021

Whoops. Completely dropped on the floor.

The good news is I have a new workflow. The bad news is I'm back in the large required course.

I'll at least put this on my laundry list, and we'll see where it goes.

@turboencabulator
Copy link
Contributor Author

@nrnrnr Ping

Also while I have your attention (and completely unrelated), do you have the NJ Machine Code Toolkit sources in a repo that could be hosted on Github? I'd hate for that project to be lost to history.

@nrnrnr
Copy link
Owner

nrnrnr commented Jun 15, 2021

This is a pretty big refactoring, and curating it is going to take
more effort than I have available for noweb. The Debian patches,
which I have long considered worth merging, are small by comparison.

From what I can tell, you've done all this work on spec. Absent clear
evidence of a problem that needs to be solved, I'm going to let this
sit until the next time I have reason to get deep into noweb... which
might be forever.

About the Toolkit, I got permission from my coauthor to put it on
github, but I haven't got there yet.

Copy link

@kwk kwk left a comment

Choose a reason for hiding this comment

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

@turboencabulator I suggest to create another PR that just contains the target simplifications from

mytarget:
  echo "goal" > mytarget

to

mytarget:
  echo "goal" > $@

This would easily be reviewable (I can help) and you can get a lot of lines from this PR in with this.

@nrnrnr what do you think?

@turboencabulator
Copy link
Contributor Author

@kwk Cherry-picking 18f5b6a would cover most of those. If you review commit-by-commit instead of looking at the total difference (as Github likes to present everything) I don't think there is too much that would be objectionable.

As a distro package maintainer, my main needs are adding DESTDIR support, and adding a generated version of toascii so it can successfully bootstrap when you don't have a copy of noweb installed.

@kwk
Copy link

kwk commented Jan 13, 2022

@kwk Cherry-picking 18f5b6a would cover most of those. If you review commit-by-commit instead of looking at the total difference (as Github likes to present everything) I don't think there is too much that would be objectionable.

@turboencabulator I agree that 18f5b6a is the one commit I was referring to. Please make a separate PR for that so we can agree on it easily and get it merge without much hassle. I'm sorry to ask for this extra work and Github is not exactly shining when it comes to handling a series of patches.

As a distro package maintainer, my main needs are adding DESTDIR support, and adding a generated version of toascii so it can successfully bootstrap when you don't have a copy of noweb installed.

I'm also interested in packaging (for Fedora and you?) which is why I've contacted @nrnrnr before but then had to work on something else.

@nrnrnr
Copy link
Owner

nrnrnr commented Jan 14, 2022

Progress sounds good! Looking forward to a smaller PR.

FYI I hate the way github manages PRs, and I do as much as I can with Magit Forge.

kwk pushed a commit to kwk/noweb that referenced this pull request Jan 31, 2022
This was originally written by @turboencabulator and part of his PR nrnrnr#10.

We firgured it is easier to review when being available as an individual
commit/PR.
@turboencabulator
Copy link
Contributor Author

@kwk Thanks for taking on the $@ changes, I've been busy over the past few weeks. Planning on breaking up the rest into manageable chunks as I get a chance. BTW I'm a Slackware user.

nrnrnr pushed a commit that referenced this pull request Feb 6, 2022
This was originally written by @turboencabulator and part of his PR #10.

We firgured it is easier to review when being available as an individual
commit/PR.
dbosk pushed a commit to dbosk/noweb that referenced this pull request Mar 6, 2024
This was originally written by @turboencabulator and part of his PR nrnrnr#10.

We firgured it is easier to review when being available as an individual
commit/PR.
dbosk pushed a commit to dbosk/noweb that referenced this pull request Mar 6, 2024
This was originally written by @turboencabulator and part of his PR nrnrnr#10.

We firgured it is easier to review when being available as an individual
commit/PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants