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

Add argo submodule and build #853

Merged
merged 34 commits into from
Oct 13, 2020
Merged

Add argo submodule and build #853

merged 34 commits into from
Oct 13, 2020

Conversation

atomb
Copy link
Contributor

@atomb atomb commented Sep 30, 2020

This adds the argo repository as a submodule as a temporary step in migrating the saw-remote-api package from the argo repository to the saw-script repository. More detail on this migration is here.

I'd like to consider this to be as close as possible to an atomic move from the argo repository, so I don't think it's necessary to review the purely added files, just those in the build and CI system that were changed to accommodate the new code.

@atomb
Copy link
Contributor Author

atomb commented Oct 7, 2020

This should not be merged until the related argo, saw-core, and cryptol PRs are merged.

@atomb atomb marked this pull request as ready for review October 12, 2020 19:48
.github/ci.sh Outdated
@@ -10,7 +10,7 @@ mkdir -p "$BIN"
is_exe() { [[ -x "$1/$2$EXT" ]] || command -v "$2" > /dev/null 2>&1; }

extract_exe() {
exe="$(cabal v2-exec which "$1$EXT")"
exe="$(cabal v2-exec which "$1$EXT" | tail -1)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm starting to think that using find directly on the dist-newstyle folder might achieve better results. It seems to be a more common practice than cabal v2-exec from other repos I've seen as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that might be the way to go. I've had endless trouble with v2-exec. I'll update it.

Copy link
Member

Choose a reason for hiding this comment

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

@atomb, I'm curious as to what problems you saw with v2-exec. The advantage of the v2-exec method over the find method is that the dist-newstyle can have multiple instances of the binary because it creates different directories based on architecture, GHC version, etc., so the find might not get the one for the current configuration (esp. with caching of builds).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One some platforms, for reasons I don't entirely understand, the output of v2-exec which ... includes extra text, and it can be hard to extract what you need. Usually, tail is enough, but it seems like more of a hack. I'm hoping that in CI the issue of multiple instances of a particular binary won't show up, but maybe I'm wrong about that.

Copy link
Member

Choose a reason for hiding this comment

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

@jared-w, is the github actions caching a potential issue for the find-based approach returning the wrong executable?

@@ -117,12 +118,12 @@ build() {
ghc_ver="$(ghc --numeric-version)"
cp cabal.GHC-"$ghc_ver".config cabal.project.freeze
cabal v2-update
cabal v2-configure -j2 --minimize-conflict-set
echo "allow-newer: all" >> cabal.project.local
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a plan of sorts to eventually not need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope so? It's really out of our control, and depends on Hackage authors not putting unnecessary upper bounds on the versions of their dependencies.

@atomb atomb merged commit d2fff60 into master Oct 13, 2020
brianhuffman pushed a commit that referenced this pull request Oct 15, 2020
brianhuffman pushed a commit that referenced this pull request Oct 16, 2020
Update stack.yaml files to work with #853.
@RyanGlScott RyanGlScott deleted the at-add-argo branch March 22, 2024 14:56
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.

3 participants