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

New build system #738

Draft
wants to merge 8 commits into
base: develop
Choose a base branch
from
Draft

Conversation

elegios
Copy link
Contributor

@elegios elegios commented May 17, 2023

This PR implements an approach for #707, that should be much more concise than the previous approach, and that additionally supports two test runners: make and tup.

The PR is presently a draft, for two primary reasons:

  • Not all special test cases are implemented yet. Most of what's mentioned in Refactoring our builds and tests #707 is implemented, but we should have some more eyes on it, as well as specifications for the remaining test cases.
    • mi tune in particular appears to not respect the --output flag, which will cause some issues
  • It has only been tested on my machine thus far, thus it's not entirely certain I correctly handle other setups. To test and see that things work on other machines I would appreciate the following:
    1. Checkout this branch.
    2. Test running make, make clean, and make test. The first two should work without issue, while the last should exhibit some test failures (feel free to look into the causes and add exceptions to misc/test-spec.mc if you have time and/or feel responsible for them, I don't necessarily have much time to look into it presently).
    3. Other make targets that could be useful to test, but aren't quite as important/different from before: install, uninstall, lint, fix, and cheat.

@elegios elegios force-pushed the new-build-system branch 6 times, most recently from b3d3fc2 to 5d6e5e1 Compare May 22, 2023 12:49
@larshum
Copy link
Contributor

larshum commented May 31, 2023

I tried running the latest version on my Mac now. The make and make clean commands worked fine, but I got a slight error for make test:

exec misc/test --bootstrapped smart
realpath: illegal option -- -
usage: realpath [-q] [path ...]
make: *** [test] Error 1

The realpath command is given a --relative-to=... flag, which isn't supported by the ancient version used in MacOS (I don't know the exact version, but the manual page is from 2011). I describe how I worked around it below. After getting that to run, I managed to get some (boot) tests to run.


To fix the problem with realpath, you can install coreutils through brew install coreutils and then you also need to update the PATH as described there to overwrite the built-in versions with more recent ones.

Commands also provided by macOS and the commands dir, dircolors, vdir have been installed with the prefix "g".
If you need to use these commands with their normal names, you can add a "gnubin" directory to your PATH with:
  PATH="/usr/local/opt/coreutils/libexec/gnubin:$PATH"

Alternatively, we could use grealpath for MacOS to avoid having to update the path.

@elegios
Copy link
Contributor Author

elegios commented May 31, 2023

Hm. It would probably be better if we could avoid adding extra dependencies if we don't really need to. In this case we know that . is inside $dir, i.e., the latter is a prefix of the former, so it might be possible to do some string manipulation in bash instead or something. I've pushed a version that does that now, could you test that it works for you as well? (ideally without the workaround you applied)

@larshum
Copy link
Contributor

larshum commented May 31, 2023

Hm. It would probably be better if we could avoid adding extra dependencies if we don't really need to. In this case we know that . is inside $dir, i.e., the latter is a prefix of the former, so it might be possible to do some string manipulation in bash instead or something. I've pushed a version that does that now, could you test that it works for you as well? (ideally without the workaround you applied)

Yes, that worked for me (without the workaround).

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.

2 participants