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

Windows Manual Installation Fix #468

Merged
merged 2 commits into from Oct 3, 2023
Merged

Conversation

Javiolonchelo
Copy link
Contributor

Fixed the manual installation instructions for Windows. I tried to copy-paste into my terminal but produced some errors. I believe there were some extra ".

I skipped the contributing guidelines because this change does not affect funcionality and I won't be working on this project in the near future.

Thanks in advance, I hope this helps :)

Fixed the manual installation instructions for Windows. I just tried to copy-paste into my terminal but produced some errors. I believe there were some extra ". I hope this change is valuable for everyone and not only for me.
Copy link
Contributor

@dschult dschult left a comment

Choose a reason for hiding this comment

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

Looks like this change should also be made on lines 96-97 and 163-164.
I’m a little surprised that the suggested change works with only two double-quotes. But the current three double-quotes is clearly wrong. I don’t have a way to test it.

@jarrodmillman jarrodmillman added this to the 1.12 milestone Jun 27, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (d2c1adb) 81.02% compared to head (a7692c8) 81.02%.
Report is 2 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #468   +/-   ##
=======================================
  Coverage   81.02%   81.02%           
=======================================
  Files           5        5           
  Lines        1254     1254           
=======================================
  Hits         1016     1016           
  Misses        238      238           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pbadeer
Copy link

pbadeer commented Jul 26, 2023

If this command is meant to work in Powershell on windows, it also needs the line ending characters changed:

python -m pip install --use-pep517 ^
              --global-option=build_ext ^
              --global-option="-IC:\Program Files\Graphviz\include" ^
              --global-option="-LC:\Program Files\Graphviz\lib" ^
              pygraphviz

rossbar added a commit to rossbar/pygraphviz that referenced this pull request Aug 1, 2023
Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

Thanks for the comments @pbadeer . Would you be able to make changes to the github workflow to use the correct powershell syntax? The relevant section is here:

# We need ``python -m pip install -U pip`` b/c Windows won't modify running programs.
- name: Install packages
run: |
python -m pip install --upgrade pip wheel setuptools
python -m pip install -r requirements/test.txt
python -m pip install --global-option=build_ext `
--global-option="-I$($env:WIN_GRAPHVIZ_DIR)\include" `
--global-option="-L$($env:WIN_GRAPHVIZ_DIR)\lib" `
.
python -m pip list

This is really the only way we have to verify that any of the suggestions actually work.

@yasu-sh
Copy link

yasu-sh commented Sep 22, 2023

@pbadeer cmd.exe 's the line continuation character is '^' . Not for PowerShell.
https://devblogs.microsoft.com/scripting/powertip-line-continuation-in-powershell/

Your command seemed to work on cmd.exe, but did not worked since --global-option

If this command is meant to work in Powershell on windows, it also needs the line ending characters changed:

python -m pip install --use-pep517 ^
              --global-option=build_ext ^
              --global-option="-IC:\Program Files\Graphviz\include" ^
              --global-option="-LC:\Program Files\Graphviz\lib" ^
              pygraphviz

When using cmd.exe as @pbadeer says, it was not successful since pep517 option is neglected by --global-options effect.

PS E:> pip --version
pip 23.2.1 from E:\\lib\site-packages\pip (python 3.10)
PS E:> cmd
Microsoft Windows [Version 10.0.19045.2846]
(c) Microsoft Corporation. All rights reserved.

E:\>python -m pip install --use-pep517 ^
More?               --global-option=build_ext ^
More?               --global-option="-IC:\Program Files\Graphviz\include" ^
More?               --global-option="-LC:\Program Files\Graphviz\lib" ^
More?               pygraphviz
DEPRECATION: --build-option and --global-option are deprecated. pip 23.3 will enforce this behaviour change. A possible replacement is to use --config-settings. Discussion can be found at https://github.com/pypa/pip/issues/11859
WARNING: Implying --no-binary=:all: due to the presence of --build-option / --global-option. 
Collecting pygraphviz
  Using cached pygraphviz-1.11.zip (120 kB)
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: pygraphviz
  **WARNING: Ignoring --global-option when building pygraphviz using PEP 517**
  Building wheel for pygraphviz (pyproject.toml) ... error
  error: subprocess-exited-with-error

  × Building wheel for pygraphviz (pyproject.toml) did not run successfully.

This case, this works on cmd.exe on the pip version 23.2.1.

PS E:\> cmd
Microsoft Windows [Version 10.0.19045.2846]
(c) Microsoft Corporation. All rights reserved.

E:\>python -m pip install --use-pep517 ^
More?               --config-setting="--global-option=build_ext" ^
More?               --config-setting="--global-option=-IC:\Program Files\Graphviz\include" ^
More?               --config-setting="--global-option=-LC:\Program Files\Graphviz\lib" ^
More?               pygraphviz
Collecting pygraphviz
  Using cached pygraphviz-1.11.zip (120 kB)
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: pygraphviz
  Building wheel for pygraphviz (pyproject.toml) ... done
  Created wheel for pygraphviz: filename=pygraphviz-1.11-cp310-cp310-win_amd64.whl size=107856 sha256=54d2
  Stored in directory: c:\users\username\appdata\local\pip\cache\wheels\5b\ee\36\aaa
Successfully built pygraphviz
Installing collected packages: pygraphviz
Successfully installed pygraphviz-1.11

@yasu-sh
Copy link

yasu-sh commented Sep 25, 2023

@rossbar I have coded and made it in successful in installation for windows cmd.exe with @pbadeer methods.

https://github.com/yasu-sh/pygraphviz/actions/runs/6294180873/job/17085663722#step:7:103

Commit of Github Actions for cmd.exe

    # We need ``python -m pip install -U pip`` b/c Windows won't modify running programs.
    # cmd.exe use same env variables with pwsh.
    - name: Install packages
      run: |
        python -m pip install --upgrade pip wheel setuptools
        python -m pip install -r requirements/test.txt
        python -m pip install --global-option=build_ext ^
          --global-option="-I%WIN_GRAPHVIZ_DIR%\include" ^
          --global-option="-L%WIN_GRAPHVIZ_DIR%\lib" ^
          .
        python -m pip list
      shell: cmd

Thanks for the comments @pbadeer . Would you be able to make changes to the github workflow to use the correct powershell syntax? The relevant section is here:

# We need ``python -m pip install -U pip`` b/c Windows won't modify running programs.
- name: Install packages
run: |
python -m pip install --upgrade pip wheel setuptools
python -m pip install -r requirements/test.txt
python -m pip install --global-option=build_ext `
--global-option="-I$($env:WIN_GRAPHVIZ_DIR)\include" `
--global-option="-L$($env:WIN_GRAPHVIZ_DIR)\lib" `
.
python -m pip list

This is really the only way we have to verify that any of the suggestions actually work.

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here - debugging via CI is not the most efficient process. Thanks to @yasu-sh and @GattoPianista for the suggestions - the proposed recipe is now confirmed to work (as indicated by the CI changes) for windows.

Closes #480 #478 #477

@MridulS
Copy link
Contributor

MridulS commented Oct 3, 2023

Thanks everyone!

@MridulS MridulS merged commit c3e862a into pygraphviz:main Oct 3, 2023
21 checks passed
rossbar added a commit to rossbar/pygraphviz that referenced this pull request Oct 3, 2023
@yasu-sh
Copy link

yasu-sh commented Oct 4, 2023

@rossbar @MridulS Thanks for your actions for this issue.
I think there are outstanding item, MacPorts and Chocolatey as I made PR once, for chocolatey, #477.

Should I made action? or Can I expect somebody in contributors be making actions?
https://github.com/pygraphviz/pygraphviz/pull/479/files#

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
8 participants