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 explanation for workaround of #3346 #3348

Merged
merged 2 commits into from
Mar 20, 2019

Conversation

wucas
Copy link
Contributor

@wucas wucas commented Mar 19, 2019

Adds an explanation for a workaround for #3346 to INSTALL.md. This way it might be easier to find an explanation if others have the same issue.

@coveralls
Copy link

coveralls commented Mar 19, 2019

Coverage Status

Coverage remained the same at 85.151% when pulling a4f5aa9 on wucas:lw/improve-INSTALL.md into 38aa4e8 on gap-system:master.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thanks! Some minor remarks attached. Is this in three commits on purpose?

INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
@wucas wucas force-pushed the lw/improve-INSTALL.md branch 3 times, most recently from 21861dd to 0bf8aa0 Compare March 19, 2019 16:43
INSTALL.md Outdated Show resolved Hide resolved
@fingolfin fingolfin added gapdays2019-spring Issues and PRs that arose at https://www.gapdays.de/gapdays2019-spring and removed gapdays2019-spring Issues and PRs that arose at https://www.gapdays.de/gapdays2019-spring labels Mar 19, 2019
Copy link
Member

@olexandr-konovalov olexandr-konovalov left a comment

Choose a reason for hiding this comment

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

Making Problems specific to Windows more visible looks good to me. I didn't know that The ^-key or "-key cannot be entered may also happen on other systems.

Looks good to me, although I can't check all macOS instructions. Note that on my Mac I have to call make with CFLAGS as below:

export CC=/usr/local/bin/gcc-7
./configure --with-readline=/usr/local/opt/readline/
make CFLAGS="-isystem /usr/local/opt/readline/include/"

Finally, would be nice to change OS X to macOS which is now the official name.

@wucas
Copy link
Contributor Author

wucas commented Mar 20, 2019

Thanks! Some minor remarks attached. Is this in three commits on purpose?

No not on purpose I just was not aware of the right workflows, I squashed them.

@wucas
Copy link
Contributor Author

wucas commented Mar 20, 2019

@alex-konovalov I've also changed OS X to macOS. For consistency I changed it in the README.md too.

@fingolfin
Copy link
Member

@wucas minor thing: your commits show up with different names. One has author Lucas Wollenhaupt <reg-github@wucas.de> and one has Lucas Wollenhaupt <github@wucas.de> Is that intentional? If you want to use both addresses, I recommend to add both to your GitHub profile, so that GitHub can correctly associate your commits to your GitHub account.

INSTALL.md Outdated Show resolved Hide resolved
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thanks, looks pretty good to me; just three minor comments, and a note on the authorship of your commits.

That said, I wouldn't complain if we merged this as-is either shrug

INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 20, 2019

Codecov Report

Merging #3348 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3348      +/-   ##
==========================================
- Coverage   85.15%   85.15%   -0.01%     
==========================================
  Files         697      697              
  Lines      344057   344057              
==========================================
- Hits       292976   292968       -8     
- Misses      51081    51089       +8
Impacted Files Coverage Δ
src/iostream.c 65.39% <0%> (-0.77%) ⬇️
src/system.c 73.5% <0%> (-0.32%) ⬇️
src/modules.c 76.53% <0%> (-0.23%) ⬇️
src/stats.c 94.93% <0%> (-0.21%) ⬇️
src/io.c 79.18% <0%> (-0.18%) ⬇️
src/objects.c 81.33% <0%> (-0.14%) ⬇️
src/plist.c 93.18% <0%> (-0.09%) ⬇️

wucas and others added 2 commits March 20, 2019 13:42
INSTALL.md: workaround for ˆ-key under OS X

change OS X to macOS
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Perfect, thank you!

@fingolfin fingolfin merged commit 304097f into gap-system:master Mar 20, 2019
@wucas wucas deleted the lw/improve-INSTALL.md branch March 23, 2019 12:46
@markusbaumeister markusbaumeister added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Apr 15, 2019
@wucas wucas added os: macos Issues and PRs that are (at least partially) specific to macOS release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 20, 2019
@wucas wucas added the topic: documentation Issues and PRs related to documentation label Aug 20, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gapdays2019-spring Issues and PRs that arose at https://www.gapdays.de/gapdays2019-spring os: macos Issues and PRs that are (at least partially) specific to macOS release notes: added PRs introducing changes that have since been mentioned in the release notes topic: documentation Issues and PRs related to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants