Skip to content
This repository has been archived by the owner on Nov 24, 2022. It is now read-only.

WIP Implement ahc-ar #663

Merged
merged 21 commits into from
May 27, 2020
Merged

WIP Implement ahc-ar #663

merged 21 commits into from
May 27, 2020

Conversation

gkaracha
Copy link
Member

Closes #649.

@gkaracha
Copy link
Member Author

@TerrorJack This is still a rough first draft but maybe it's time to have a look, tell me if I am going the right way about it? It seems to be working just fine on my Ubuntu, but I have no idea if it would really work on darwin (e.g. fix issue #345).

@gkaracha
Copy link
Member Author

Nah, scrap that. I missed an error from getGNUArchEntries due to name truncation. I'll fix that first and we can discuss options later.

@ProofOfKeags
Copy link

I can test on Darwin if you want to fish for what issues you might run into.

@gkaracha
Copy link
Member Author

I can test on Darwin if you want to fish for what issues you might run into.

Ah, that'd be great! I will ping when the patch is ready for testing.

@gkaracha
Copy link
Member Author

gkaracha commented May 26, 2020

OK, @TerrorJack you can have a look now. We still have to decide how much we want to deviate from what GNU ar does. The patch currently

  • just ignores all arguments but the first .a file and the list of .o files. Shall we warn or do something with the rest?
  • does not parse quoted arguments correctly (e.g. "this is a filename with spaces in it.o"), it uses words to space-separate the arguments. Do we need that?
  • parses @file arguments recursively (as specified by man ar). Do we need that behavior?

@TerrorJack
Copy link
Member

TerrorJack commented May 26, 2020

@gkaracha

@TerrorJack
Copy link
Member

One place we forgot to modify: the boot.sh boot script compiles libraries in two ways: either calling ahc-cabal, or calling a Setup program and completing the configure/build/install steps manually. This PR only affects ahc-cabal so far; we also need to modify Asterius.Boot and add --with-ar=<path to ahc-ar> to the default configure options.

@gkaracha
Copy link
Member Author

@TerrorJack

see https://haskell-code-explorer.mfix.io/package/Cabal-2.4.1.0/show/Distribution/Simple/Program/ResponseFile.hs#L34 for how the response file is generated.

Nice, thanks for the source. This means that we have to undo the effects of escapeResponseFileArg too, right?

@TerrorJack
Copy link
Member

I don't think there's need to unescape object file paths for now. The bizzare object file names have never occured in the field. If later we encounter a crash in third party libraries we can fix this up.

@TerrorJack
Copy link
Member

@gkaracha Please cherry pick 8bd5b82 on this branch and re-test. For unknown reasons, --with-ar= must be hard-wired in boot.sh. If you add -v3 to configure options you can show that the system ar is still used previously.

@gkaracha
Copy link
Member Author

@gkaracha Please cherry pick 8bd5b82 on this branch and re-test.

Done: https://buildkite.com/tweag-1/asterius/builds/609. Fingers crossed.

@TerrorJack
Copy link
Member

@gkaracha And also 0085dcf. The patch just gives up the reparsing/combining logic of object files.

The reason for the patch is a segmentation fault in macOS booting as shown in CI. I suspect there might have been an OOM condition, and giving up object recombining seems to make ahc-ar work on macOS. (Although it's failing for something else now)

@TerrorJack
Copy link
Member

I apologize for the "good first issue" tag in #649 btw. I just removed it. 🤦

@gkaracha
Copy link
Member Author

@gkaracha And also 0085dcf. The patch just gives up the reparsing/combining logic of object files.

Yes, I think that's a good idea in general, it is a change in logic we best include in a separate PR.

The reason for the patch is a segmentation fault in macOS booting as shown in CI. I suspect there might have been an OOM condition, and giving up object recombining seems to make ahc-ar work on macOS. (Although it's failing for something else now)

Hmmmm, something similar is blocking #622 (it also OOM for 2 or more cores). If/When we find the source of this I will revisit #622 to see if it's unblocked.

I apologize for the "good first issue" tag in #649 btw. I just removed it.

No worries about that! (but still, thanks for removing 😛)

@gkaracha gkaracha marked this pull request as ready for review May 26, 2020 16:21
asterius/app/ahc-ar.hs Outdated Show resolved Hide resolved
@TerrorJack
Copy link
Member

@ProofOfKeags This PR solves the ar problem on macOS, but not sufficient to fully fix macOS support; there's another error as in https://app.circleci.com/pipelines/github/tweag/asterius/1697/workflows/97316b3c-006c-4e78-9e40-c8e14560563f/jobs/13829.

@TerrorJack TerrorJack merged commit a5c377f into master May 27, 2020
@TerrorJack TerrorJack deleted the wip-fix-issue-649-ar branch May 27, 2020 10:24
@ProofOfKeags
Copy link

@TerrorJack Do you know the cause of the new error? or does that need to be investigated?

@TerrorJack
Copy link
Member

@ProofOfKeags See #674 (comment). It'll take some time to figure out the solution; right now a possible workaround is adding gnu gcc to replace the xcode stuff in PATH in the shell which you call asterius, but I don't have time to test whether it'll actually work.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement ahc-ar
3 participants