Out-of-Starlink build support including running Fortran tests and basic CI - #37
Out-of-Starlink build support including running Fortran tests and basic CI#37embray wants to merge 62 commits into
Conversation
|
You can inspect a successful CI run here: https://github.com/embray/ast/actions/runs/23855735181 |
|
Thank you! I was actively pondering looking into cmake for a GitHub Action build but I'm really happy you did this. At Rubin we are pondering trying to rewrite the Fortran tests into C to make life a bit easier for us (and maybe getting an LLM to help with that). |
|
A cmake-based build would not be a bad idea. I'm still partial to old school autotools, but I get comments on various other projects from people requesting a cmake build system as well, and it has its advantages (and disadvantages). For now I was hoping to just maintain the status quo while providing more flexibility :) |
|
@embray Sorry if I messed you up by doing the F to C test migration and using cmake. A starlink-less autotools build seems like it might be useful in general, especially if that means that someone can do a Testing the fortran code seems like a good thing in general but is less critical now we have ctest + asan + coverage in a GitHub action now. I have done the K&R fixes so you can drop those from this branch when I merge (hope I can do that very soon). Maybe we should vendor messgen and use it for the cmake build as well. I wonder if I don't think I mind either way as to keeping this PR or not. I feel bad that you spent a lot of time getting this to work. |
|
No worries, it's my bad for just bumbling ahead without coordinating first. I'm glad it spurred you into doing all that other work, and there's still a lot here that can be useful I think too. I'll wait until #38 is done and then see how to reintegrate. |
| character path*255 | ||
|
|
||
| open( 10, status='old', file=file ) | ||
| call getenv( 'srcdir', srcdir ) |
There was a problem hiding this comment.
Note on this: GCC provides getenv as an extension so this works fine. If for some remote reason someone couldn't use this, there is also a psx_getenv defined in Starlink's psx library that could easily be reimplemented in my psx_standalone.c But it seems unnecessary.
There was a problem hiding this comment.
I think we use gfortran on macOS as well so it shouldn't be an issue but PSX_GETENV is the Starlink standard as you say so maybe it is better to copy that code over to psx_standalone.c since that file already exists.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #37 +/- ##
=======================================
Coverage 61.41% 61.41%
=======================================
Files 83 83
Lines 96474 96474
Branches 30586 30586
=======================================
+ Hits 59249 59250 +1
+ Misses 21067 21066 -1
Partials 16158 16158 ☔ View full report in Codecov by Harness. |
8bfabca to
3366081
Compare
This addresses the question in PR #37: "One thing I have discovered is that the serialization tests fail by default because Claude decided that this was a C99-compatible repo and forces C99 standard. This is fine as such but we then run into a problem with AST_DBL_DIG being 18 in C99 mode (DBL_DIG [15] + 3) because DBL_DECIMAL_DIG does not formally exist in the C standard until C11. With C11 you get 17 digits and we get reproducible numbers in the output files for string comparisons." @dsberry's preferred fix (which I also agree is less disruptive) to set the fallback to 17 digits when compling for C99 resulting in consistency regardless of which C standard was compiled against; this fixes all the remaining test failures for me.
|
@timj I rebased this on master and made several small improvements, including incorporating more of the new test fixtures (though not all of them yet). I also make several fixes so that Remaining open question as to whether it's worth the effort to maintain two parallel testing systems. A lot of work goes into integrating the same tests into automake as into cmake. Though now that it's mostly done for most of the hard cases anyways, maybe just leave it for now. I did not yet bring in some of the newer tests, including the oracle tests. Could be left as a follow-up. Otherwise this seems to be working well, with or without the Starlink build system. |
|
Probably need to edit the README to describe this.
I guess in theory we could live in a world where |
I was thinking the same. At this point I'd be a bit sad to remove the tests I already added too, but perfectly willing if we want to slim it down. At least I finally learned how to use the mysterious TAP protocol (every time I looked at that section of the automake manual my eyes glazed over, but actually it's pretty easy). I'll add something to the README. I still find |
|
I'm okay with leaving the tests you have made in but maybe we should say that we are not required that every ctest test is also integrated into autotools. Treat ctest as the full suite going forward since it also provides the code coverage. |
- Starlink/ast#37 - Starlink/ast#51 - Starlink/ast#66 - Starlink/ast#67 This includes the experimental SIMD support (enabled by default, and support in AST for libfyaml, allowing us to drop the libyaml requirement). Also updates the minimum supported versions of the ASDF tags supported by AST.
Moves the YAML backend detection into a dedicated macro in m4/ast_yaml.m4 `AST_CHECK_YAML` This handles things like setting precious variables (F)YAML_CFLAGS, (F)YAML_LIBS consistently between backends; keeping some of the messier logic directly out of configure.ac. Pulled this in from a branch that tested merging this with my autotools branch for PR Starlink#37. As a follow-up we might want to consider adding optional pkg-config support here, which would make our lives easier. The new cmake build system already uses pkg-config. I just resisted adding it for now to the autotools build since there was no prior precedent of its use, and I worry about unintended breakage of the building within Starlink.
Moves the YAML backend detection into a dedicated macro in m4/ast_yaml.m4 `AST_CHECK_YAML` This handles things like setting precious variables (F)YAML_CFLAGS, (F)YAML_LIBS consistently between backends; keeping some of the messier logic directly out of configure.ac. Pulled this in from a branch that tested merging this with my autotools branch for PR Starlink#37. As a follow-up we might want to consider adding optional pkg-config support here, which would make our lives easier. The new cmake build system already uses pkg-config. I just resisted adding it for now to the autotools build since there was no prior precedent of its use, and I worry about unintended breakage of the building within Starlink.
…k build tree, esp FORTRAN tests - Drop requirement for (rather old) starlink fork of automake - Brought in my own replacements for the bare minimum set of routines from Starlink libraries needed for the tests; written in C which I'm more comfortable with, but with gfortran name mangling applied, following the conventions from the macros in f77.h; some of this is probably still buggy but the tests pass - Bring the Fortran tests more into the automake build system
…ed tests add a Makefile.am in ast_tester and move fortran test definitions there, and hook them properly into automake's test runner Now there are two test suites: ast_tester/ and ast_tests/ I believe this will still all work the same otherwise when building in the Starlink environment but this needs to be confirmed.
The stub took $1 as the variable name and $2 as the list of programs to look for, which is the reverse of the real starconf macro ($1 is the program list, $2 an optional Starlink subdirectory). So `STAR_CHECK_PROGS(star2html)` looked for nothing and set no variable, and `STAR_CHECK_PROGS(prolat, sst)` searched for a program called "sst". Nothing depended on the result until now, so this was invisible. Follow the real interface instead: upper-case each program name into its own variable and default it to the bare name, so a caller can tell "found" from "not found" with `test -x` exactly as with the real macro. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Typesetting SUN/210 and SUN/211 needed starlink.cls and the three style
files it requires, which came from $STARLINK/share/latexsupport by way of
an undocumented STARLATEXSUPPORT environment variable. Nothing set it,
so `make dist` silently shipped no support files at all and a release
tarball's documents could not be recompiled. Vendor them instead: they
pull in nothing but standard TeX Live packages, so a plain TeX Live plus
these four files builds the PDFs with no Starlink installation.
The copies are byte-identical to upstream and are meant to stay that way,
so AST's own additions live in a new ast.sty. It defines three macros
that were undefined in every build to date:
- \htmlref, which addlinks emits for every cross-reference, is defined
only in starstyle.4ht for the tex4ht hypertext build. With nothing
to define it, pdflatex reported 6534 undefined-control-sequence
errors and exited non-zero, and TeX skipped the macro but still
typeset its arguments -- so every cross-reference appeared twice in
the PDF.
- \htmladdnormallink and \htmladdnormallinkfoot are LaTeX2HTML macros
from html.sty, which nothing loads, though sun_master.tex uses both
and starabbrev.sty's \FITSref is written in terms of the first.
These are PDFs and starlink.cls loads hyperref, so render them as
real links rather than dropping the URL as html.sty does for print.
The documents now build with pdflatex reporting no errors at all, and
each cross-reference appears once.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The PDFs now need only TeX Live plus the vendored LaTeX support files, but the .htx_tar hypertext products still need the Starlink star2html program. Requiring it unconditionally means a build without a Starlink tree -- a GitHub Actions runner, say -- cannot produce documentation at all, even though the PDF is what nearly every reader wants. Include the .htx_tar targets only when star2html is actually present, and warn when it is not: a full release has historically always carried the hypertext, so its absence should be visible rather than silent. --disable-hyperdocs skips them even where star2html exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
.gitignore already covers the other pdflatex byproducts for both documents (.lof, .log, .out, .toc) but not .aux, so building the docs left two untracked files in the tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The three LaTeX2HTML macros AST uses are defined in LaTeX2HTML 95.1's
html.sty, a copy of which sits in $STARLINK/bin/sst. Cite it rather than
reasoning from first principles, and say exactly where we depart from it:
\htmlref #1 -- as html.sty
\htmladdnormallink #1 -- widened to \href
\htmladdnormallinkfoot #1\footnote{#2} -- widened to \href
\htmlref is byte-for-byte html.sty's definition, so the earlier choice to
render the text and drop the label was right for a better reason than the
one recorded: it is what these documents were written against. The two
link commands drop or plain-print the URL because html.sty targets paper;
these are PDFs with hyperref loaded, so both become live links. Nothing
printed changes either way.
AST uses no other macro from that family -- \htmlref 3248 times,
\htmladdnormallinkfoot 4, \htmladdnormallink 2, and nothing else -- so
three definitions here beat vendoring all 231 lines of html.sty, which
carries no licence statement of its own.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The standalone stub for STAR_CNF_F2C_COMPATIBLE inferred f2c compatibility from the compiler's identity -- "gfortran is f2c-compatible" -- and so set REAL_FUNCTION_TYPE to double. That is backwards: gfortran uses the f2c calling convention only when given -ff2c, and the real Starlink macro does not guess, it compiles a C function returning a float, calls it from Fortran as a REAL function, and reads the answer off what comes back. On this machine the real macro and the CMake build both said float while the standalone build said double. AST itself never uses F77_REAL_FUNCTION, so nothing in the test suite could notice, but f77.h is an installed header: a downstream Fortran/C caller built against a standalone AST got a mismatched return ABI. Port the real probe rather than correcting the constant, so the answer stays right for a compiler that genuinely is in f2c mode, and run the same probe from CMake, which had the value hard-coded. Verified in both build systems: default gfortran gives float, FCFLAGS=-ff2c gives double. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
STAR_MESSGEN substitutes a variable called MESSGEN, and Makefile.am
assigned to the same name to point the standalone build at the messgen it
builds from star_compat/messgen.c. A Makefile.am assignment overrides
configure's substitution, so every Starlink bootstrap reported:
Makefile.am:881: warning: MESSGEN was already defined in condition
TRUE, which includes condition !STARLINK_BUILD ...
Harmless -- the assignment is inside !STARLINK_BUILD, so it never actually
displaced the value a Starlink build wants -- but it is noise on every
bootstrap and an accident waiting to happen. Use AST_MESSGEN instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The CMake build required a 64-bit target before choosing size_t for the hidden Fortran string length type; neither the Starlink macro nor the standalone stub does. On a 32-bit target size_t is int-sized anyway, so the extra condition bought nothing and only made CMake disagree with both autotools builds. Drop it: gfortran 8 and later use size_t regardless of word size. Both now emit an identical src/f77.h here (size_t, and float for F77_REAL_FUNCTION_TYPE), matching the Starlink build. Also record, at both sites, the one place they still differ from the real macro: it uses long for 64-bit Intel and Sun Fortran and probes anything else non-GNU, where these give every non-GNU compiler int. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three EXTRA_DIST blocks enumerated files whose real membership rule is "everything of this kind", so each had to be extended by hand every time master added a file, and nothing enforced it. Both omissions found while rebasing this branch were exactly that: cmake/run_simplify_noop_test.cmake and ast_tester/testimmutable.c each produced a tarball that configured and then failed to build, discovered only after a full distcheck. Two changes, because either alone leaves a gap. First, express the rule instead of the list. Every file in cmake/ is a build input, and every C, Fortran and header source in ast_tester/ is compilable by one build system or the other, so ship those by pattern from the dist-hook -- the same find-based approach already used for the fixture tree. Not $(wildcard) in EXTRA_DIST: that is GNU-make-only and automake reports it as non-portable. This also picks up ten sources that were already missing, among them the legacy Fortran programs and the cases.awk and plotcase.awk manifest helpers. Second, check the result. dist-check-tracked, run as the last step of the top-level dist-hook -- after automake has staged the subdirectories -- asks git for every tracked file under cmake/ and ast_tester/ and fails if one did not reach the tarball. It checks all of them rather than the extensions the patterns match, so it catches a kind of file nobody anticipated; verified against both a stray .cxx and a removed cmake helper. The exclusions are stated as exclusions on purpose: not shipping something becomes a decision rather than an oversight. The check needs a git checkout, so it guards `make dist' rather than guaranteeing anything about an arbitrary tarball. That is where dist is run. The tarball's contents are otherwise unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Making the .htx_tar products conditional on star2html left a warning firing on every standalone configure -- but a standalone build has an empty STAR_LATEX_DOCUMENTATION and produces no documentation at all, so telling its user we are "building PDF documentation only" is misleading noise. Gate the warning on starconf being present, which is the case where the documentation is real. A standalone configure log is quiet again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three changes to how CI runs, and one new step. All three workflows only fired on pushes to master and pull requests targeting master, so pushing a topic branch ran nothing at all -- and autotools.yaml does not exist on master, so not even workflow_dispatch was offered for it. Check every branch push as well as every pull request. The pull_request event stays because a fork's push never reaches this repository, so it is what covers external contributions; both events report a status on the head commit, so either can be a required check in branch protection. For a branch here with an open pull request that does mean two runs of one commit, which GitHub gives no clean way to avoid; the new concurrency group at least cancels superseded runs, though not on master where the completed record is worth keeping. The autotools workflow also passed only F77 to configure. Automake compiles .f through F77, but configure's Fortran probes -- including the f2c one that decides F77_REAL_FUNCTION_TYPE -- go through FC, which was left to autodetection and would find whatever Fortran the runner image happens to carry. Set both. gfortran is now installed explicitly on Ubuntu rather than relied on from the image, as cmake.yaml already does, and checkout moves to v6 to match the other two workflows. Finally, build the distribution tarball with CMake here. That check used to live inside `make distcheck', which meant cutting a release required CMake as well as the autotools -- backwards, since nothing CMake needs is generated at distribution time. CI has both toolchains, so it belongs here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three files reached the tarball that configure generates from templates beside them, so the shipped copies described whoever ran `make dist': src/f77.h @TRAIL_TYPE@, @REAL_FUNCTION_TYPE@, @BLANK_COMMON_SYMBOL@ src/object.h @threads@ ast_link_adam @EXTERNAL_PAL@, @EXTERNAL_CMINPACK@, @yaml@ The headers arrive by way of libast's source list, whose order is load-bearing for the ast.h rule, so they are dropped from the tarball in the dist-hook rather than by restructuring that list. ast_link_adam was marked dist_bin_SCRIPTS while its sibling ast_link was not, though both are AC_CONFIG_FILES outputs -- a stale marker from before they became derived, which the comment above it already argued against. src/version.h stays: its rule is guarded by @predist@, so an unpacked tarball cannot remake it. With that, `make distcheck' no longer runs a CMake build. Requiring CMake in order to cut a release was backwards -- CMake regenerates everything from CMakeLists.txt, so unlike the autotools half-built state there is nothing it needs generated at distribution time. The check moved to CI, which has both toolchains. To keep what it was uniquely catching, dist-check-tracked now covers src/ as well: the CMake-only sources there are enumerated in EXTRA_DIST, so a new one would otherwise have slipped past both. Also removes a dead CMAKE_EXTRA_FLAGS knob that was never defined, and a .PHONY naming a target (distcheck-make) that never existed. Verified: the tarball still builds and passes 1251 CMake tests, and distcheck passes with no CMake involvement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@embray I have rebased this ticket and made it work with the new test fixture organization. I would like to get it merged so that we have the possibility in the future to make automated releases by having git tags triggering source distribution packages to be uploaded to GitHub. The wrinkles are:
I'm not sure if the prolat problem is a deal breaker. We could build the PDF separately and add it manually to the GitHub release artifacts. To make it work properly I'd probably write a little prolat replacement in python and pip install it into the CI build. @dsberry I assume you would rather I included the PDF? I don't think including the HTML is worth doing since star2html/latex2html is very outdated now. I'd be better off porting sun_master.tex to typst. We do need to make a release but I can do that manually for now (but I'd still like to merge this PR). |
|
One other thing: currently the |
|
This is Claude's analysis of the situation: Starlink-free GNU Autotools buildLets the autotools build run without Starlink's 62 files changed, +5566 / −314. Rebased onto master (
|
| default gfortran | FCFLAGS=-ff2c |
|
|---|---|---|
| autotools standalone | float |
double |
| CMake | float |
double |
The documentation build had been broken for a long time. \htmlref — which AST's own addlinks writes for every cross-reference — is defined only in starstyle.4ht, which tex4ht loads for the hypertext build. The pdflatex path had nothing to define it, and since TeX skips an undefined macro but still typesets its arguments, every cross-reference was printed twice. \htmladdnormallink and its footnote variant were undefined in every build, hypertext included. 6534 errors and a non-zero pdflatex exit, before:
before after
undefined macros 6534 0
LaTeX errors 6534 0
make exit status 2 0
sun210.pdf 267 pp 266 pp <- duplicate refs gone
sun211.pdf 274 pp 273 pp
Releases shipped no LaTeX support files at all. starlink.cls and friends came from $STARLINK/share/latexsupport via an undocumented STARLATEXSUPPORT environment variable that nothing set, so the dist-hook meant to copy them copied nothing — a release cut from this tree had documents that could not be recompiled. The four files are now vendored in star_compat/latexsupport/, byte-identical to upstream with provenance and SHA-256s recorded; AST's own additions live in a separate ast.sty, whose three macro definitions come from LaTeX2HTML 95.1's html.sty. STARLATEXSUPPORT is gone.
STAR_CHECK_PROGS had its arguments reversed in the stub, so STAR_CHECK_PROGS(prolat, sst) searched for a program called sst and STAR_CHECK_PROGS(star2html) set nothing. Invisible until the optional hypertext work needed STAR2HTML.
Distribution completeness. Three EXTRA_DIST blocks enumerated files whosereal rule is "everything of this kind", so each had to grow whenever master added one — and nothing enforced it. Both files that broke distcheck during this rebase (ast_tester/testimmutable.c, cmake/run_simplify_noop_test.cmake) were exactly that. Now cmake/ ships as a tree, ast_tester/*.{c,f,h} ship by pattern, and dist-check-tracked asks git for every tracked file under cmake/, ast_tester/ and src/, failing make dist immediately if one did not arrive. Verified against both a stray .cxx and a removed CMake helper. This also picked up ten sources that were already missing.
Configure-generated files were being distributed. src/f77.h, src/object.h and ast_link_adam all reached the tarball carrying the substitutions of whoever ran make dist — the Fortran ABI types, @THREADS@, and the --with-external-pal / --without-yaml choices respectively. Only the .h.in templates ship now. src/version.h deliberately stays: its rule is @PREDIST@-guarded, so an unpacked tarball cannot remake it.
Smaller ones. MESSGEN in Makefile.am shadowed configure's own substitution, which Starlink's automake reported on every bootstrap. CMake's TRAIL_TYPE required a 64-bit target where neither autotools build does, so the two now emit an identical src/f77.h. A .PHONY named a target that never existed, and CMAKE_EXTRA_FLAGS was referenced but never defined.
make dist no longer needs CMake
The CMake-from-tarball check used to run inside make distcheck, which meant cutting a release required both toolchains. Nothing CMake needs is generated at distribution time — unlike the autotools half-built state, it regenerates everything from CMakeLists.txt — so that check moved into CI, where both toolchains are present anyway.
Verification
| Check | Result |
|---|---|
make check (standalone autotools) |
732 / 733 pass, 1 skip (testhuge), 0 fail |
ctest (CMake, Debug and Release) |
1251 / 1251 |
make distcheck |
passes, autotools only |
CMake build of the make dist tarball |
1251 / 1251 |
make sun210.pdf sun211.pdf |
exit 0, zero LaTeX errors |
| Data-driven case counts, both suites | 192 wcsconv, 445 simplify, 20 grid |
The two make dist tarballs — one built with the Starlink tools, one without — differ by 11 entries out of ~1951, every one accounted for: six documentation products and three messgen outputs on the Starlink side (both by design), and m4/starlink_compat.m4 plus README.md on the standalone side. That last one is a toolchain artefact, not a repository one: automake 1.18 auto-distributes the .md variants of standard files and automake 1.15.1-starlink does not, so the two tarballs cannot be byte-identical while the toolchains differ in version — worth knowing before anyone writes a comparison into CI.
Verified on macOS 26.6.2 / Darwin 25.6.0 (arm64), Apple clang 21.0.0 and gfortran 16.1.0, automake 1.18.1 standalone and 1.15.1-starlink with autoconf 2.69 for the Starlink build.
Notes for review
- There are 6
fixup!commits on the tip, named for the commits they belong to. They wantgit rebase --autosquash --interactive masterbefore this merges. cmake.yamlandcoverage.yamltrigger changes are outside this branch's nominal scope. All three workflows previously ran only on pushes to master and PRs targeting master, so pushing a topic branch ran nothing; they now check every branch push and every pull request, with aconcurrencygroup to cancel superseded runs. Consistency across the three seemed worth it, but say the word and they can be split out.- Gating merges on these still needs the check names added as required status checks in branch protection; the matrix jobs report individually.
Deliberately not in this PR
- A Python replacement for Starlink SST's
prolat, whichgetattshells out to. Until then a documentation build from a bare checkout is not possible, so the LaTeX half being Starlink-free is the prerequisite rather than the whole job. - Automated releases (tag → build → upload). Everything upstream of the upload works; the release job itself does not exist yet.
- Running every test under both build systems. 12 tests and the 152
simplify_*_noopidempotence checks remain CMake-only, which is intentional for now — their sources reach the tarball, which is what matters. - The duplicate, byte-identical
ast_test.cat the repository root and inast_tester/. TRAIL_TYPEfor non-GNU compilers: the real macro useslongfor 64-bit Intel and Sun Fortran and probes anything else, where both of these give every non-GNU compilerint. Recorded in a comment at both sites.
CMake cannot run target executables without a configured emulator. Assume the normal float return convention in that case, matching the Autotools cross-compilation fallback. Co-authored-by: Codex <codex@openai.com>
The test always fails when AST is configured without YAML. Only build and schedule it when YAML support is enabled. Co-authored-by: Codex <codex@openai.com>
Out-of-source builds must use headers generated for that build. Fall back to source-tree headers only when no build copy exists. Co-authored-by: Codex <codex@openai.com>
Close the Fortran conditional before shared distribution rules. Schedule the C data tests and huge-test wrapper in C-only builds. Co-authored-by: Codex <codex@openai.com>
|
@timj About the tests, previously we talked about maybe having a reasonable subset of tests as a smoke test:
For the dist tarball it could include only the files needed for that subset of tests, which I could also work to formalize a bit better, such that About the PDF doc one possibility would be to have it built and saved in git. Yes, this is a bit of an anti-pattern but for docs I tend to make an exception, especially with latex since the output can depend a lot on what fonts, packages, and their versions are available on the system that renders it to PDF. For some packages I've had man pages that are generated from Sphinx documentation (after all, who wants to manually write *roff?) Rather than requiring Python + Sphinx and associated tools just to build my distribution, I keep the generated man page checked into the repository as well, and just periodically update it when necessary. Then including it in |
| if !NOYAML | ||
| check_PROGRAMS += testyamlchan | ||
| TESTS += testyamlchan | ||
| endif |
There was a problem hiding this comment.
This will conflict with #67 which handles this differently but no problem for now.
| my $path = "$srcdir/$file"; | ||
| $path = "$builddir/$file" if ( ! -f $path && -f "$builddir/$file" ); | ||
| my $path = "$builddir/$file"; | ||
| $path = "$srcdir/$file" if ! -f $path; |
There was a problem hiding this comment.
Oops--yes that's better.
| dnl double-returning one reads the float's bits as a double and gets something | ||
| dnl else, which the test detects by the result comparing equal to zero. That | ||
| dnl is indirect, but it is the original's test and it distinguishes the two | ||
| dnl conventions in practice. |
There was a problem hiding this comment.
Ah, this looks like something Claude wrote. I was confused in #37 (comment) if "Bugs found and fixed along the way" were things I had fixed, or things it fixed while rebasing on master...
Here there is definitely a degree of subtlety I was not aware of. I just know enough Fortran to get as far as I did, not these kinds of nitpicks. But I think that might explain why I was seeing some compiler warnings about this.
There was a problem hiding this comment.
OK I see now the full version of this is verbatim (other than the comment) out of the original starconf.m4 . That reminds me that back when I was doing this originally I just started out with all those macros as stubs (or even missing) and tried to reverse engineer minimal stand-ins where required. Here it's clear that the full version is more robust and makes fewer assumptions. I'm surprised by the R .NE. 0.0 check as opposed to just writing R .EQ. 1.0 but I'm not sure off my head how well defined it is if a float is cast to a double/REAL.
I wrote a new package https://github.com/Starlink/starprolog that replaces the Fortran prolat command and the AST perl getatt so I think I can now make the docs in the action. |
2 participants
Summary
This PR does a few things:
make check, rather than going throughDetails
I'm always wary of touching someone else's build system, but I tried to keep this as non-invasive as possible. This came about because when I started working on AST I didn't have the main Starlink project built on my system, so I had to make a few small local patches to get anything to build.
This is an effort to clean up and formalize those patches, while trying not to break the existing in-Starlink build. The two build systems don't fully co-exist. Instead one starts by calling
bootstrap.localwhich creates its ownconfigurescript. The originalconfigure.acis mostly unmodified, but stubs are provided for theSTAR_macros that are used. Also had to import a copy of the messgen.c program.Also uses a bit of a hack to only require the -starlink version of automake if
starconfis detected on the path.Tests
I wanted to see if I could build the test suite via automake as well, including the Fortran tests, in the out-of-Starlink build. This requires cherry-picking small bits of some of the Starlink libraries, which I put in
ast_tester/star_compat/. This was just done by trial-and-error until I had all the pieces that happen to be used by the tests. In the process I rewrote several of the utility functions in C, partly to learn how they work and partly because I'm just more comfortable in C. All of them are pretty straightforward, except maybe for thepsx_mallocstuff. This could be bug-prone since I hard-coded the size of the pointer table to 4096, which seems good enough for now. Could change it easily enough to be dynamically resized as needed.With these small pieces out Starlink I can successfully build and run the tests with
make check.I never bothered to run testhuge.f to completion (i haven't even looked at what's in it). It's skipped by default but can be enabled by running
make check TEST_HUGE=1.Misc
Adds a minimal CI configuration that tests the standalone build on Ubuntu and macOS. For the Mac case, although it comes with clang out of the box from Xcode it doesn't have a Fortran compiler. I decided to give flang a try (sticking within the LLVM ecosystem) and it seems to work.
Also found via the macOS build that recent versions of autoconf will try to automatically detect the highest C standard supported by your compiler, and will add it to the CFLAGS (e.g.
-std=gnu23) unless you explicitly state otherwise. This broke compiling some code (esp. in the vendored wcslib) that still used K&R style function definitions, which are no longer supported in C23. Rather than fight autoconf on this I decided it would be worthwhile to just reformat those functions to modern style.