Skip to content

Conversation

@james-d-mitchell
Copy link
Contributor

This PR tries to resolve Issue #97 by detecting whether or not tar is gnu-tar. Maybe there's a better way of doing this, happy to update as necessary.

I read your comment @fingolfin:

However, I think the real problem here is that PKGMAN_Exec deliberately merges stderr and stdout, which breaks here. I have my doubts that this is a good idea in general; it definitely is not here. Perhaps PKGMAN_Exec could get an optional argument which can be used to turn this feature off.

I'm not sure why this is the real problem in this case, the outcome here is more or less the same whether or not the 2&>1 is included, maybe I'm missing something. "More or less" mean that the installation still fails but the error is just displayed in the terminal instead of being captured in the record that's output by PKGMAN_Exec.

I think it would be better to never merged stderr into stdout and instead redirect it into a log file.

Also I'm not sure how this can be accomplished in practice using Process within PKGMAN_Exec, since it doesn't appear to provide a facility for capturing stdout and stderr separately.

@codecov
Copy link

codecov bot commented Sep 20, 2022

Codecov Report

Merging #99 (8192e55) into master (74afdca) will increase coverage by 0.18%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master      #99      +/-   ##
==========================================
+ Coverage   82.91%   83.10%   +0.18%     
==========================================
  Files           4        4              
  Lines        1001     1006       +5     
==========================================
+ Hits          830      836       +6     
+ Misses        171      170       -1     
Impacted Files Coverage Δ
gap/PackageManager.gi 81.70% <83.33%> (+0.20%) ⬆️

@mtorpey
Copy link
Collaborator

mtorpey commented Sep 21, 2022

Thanks for this @james-d-mitchell! I think I agree that this doesn't fix the underlying problem, but for now I'm glad this makes things better.

@mtorpey mtorpey merged commit 2df789a into gap-packages:master Sep 21, 2022
@fingolfin
Copy link
Member

To explain what I meant: This problem would not exist if --warning=none was not used (I never needed to use it before and didn't even know not exists).

So just dropping it would fix things, except that of course this would cause another issue to resurface. And that issue in turn is caused by merging stdout and stderr.

and yeah, GAP's Process function does not support stderr. That's a major limitation that should be fixed in GAP... see gap-system/gap#4657 (I just put that on the 4.13 milestone)

@james-d-mitchell james-d-mitchell deleted the fix-issue-97 branch September 22, 2022 08:15
@james-d-mitchell
Copy link
Contributor Author

Aha @fingolfin, now I understand what you mean, so a proper fix to this would be to use Process when it can separate stdout and stderr, and not use the flag --warning=none, but log the output of stderr to a file instead. Very good, I like that infinity times better than the hack in this PR.

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