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 template files to package #347

Merged
merged 1 commit into from
Jun 7, 2024
Merged

Add template files to package #347

merged 1 commit into from
Jun 7, 2024

Conversation

kvid
Copy link
Collaborator

@kvid kvid commented May 16, 2024

Specify all HTML files under templates folder to be included as package data files.

Is this sufficient to fix #345? Some sources claim that include_package_data=True also is needed, but preliminary testing at my Windows setup seems to prove otherwise. I would prefer that some others could confirm at different platforms and versions.

Please, anyone, test this branch at the platform(s) you have access to, and report back here the output from all commands. See the simple installation command in #347 (comment) below.

Alternatively, a longer and more complicated test procedure is suggested a bit earlier below, and if you don't have git installed, you can replace the git clone command with unpacking all files from this ZIP-file into a new subfolder you call WireViz using your favorite unzip tool. The subfolder is called WireViz-fix345 inside the ZIP-file, but you can rename it after unpacking.

  • Verified at Windows
  • Verified at Linux
  • Verified at macOS

Specify all HTML files under templates folder
to be included as package data files.
@kvid
Copy link
Collaborator Author

kvid commented May 19, 2024

Suggested test procedure:

cd {some empty folder}
python3 -m venv .venv
source .venv/bin/activate
git clone https://github.com/wireviz/WireViz --branch=fix345
cd WireViz
pip install .
cp examples/demo0?.yml ..
cd ..
rm -rf WireViz
wireviz demo0?.yml
which wireviz
pip -V
python -V
uname -a

or at Windows:

cd {some empty folder}
python3 -m venv .venv
call .venv/Scripts/activate.bat
git clone https://github.com/wireviz/WireViz --branch=fix345
cd WireViz
pip install .
copy examples/demo0?.yml ..
cd ..
rmdir /s /q WireViz
wireviz demo0?.yml
where wireviz
pip -V
python -V
ver

@kvid kvid linked an issue May 25, 2024 that may be closed by this pull request
@kvid kvid mentioned this pull request May 25, 2024
15 tasks
@kvid kvid changed the base branch from master to release/v0.4.1-rc May 25, 2024 16:38
@kvid kvid requested a review from formatc1702 June 1, 2024 01:27
@kvid kvid added the help wanted Extra attention is needed label Jun 1, 2024
@kvid kvid requested a review from amotl June 4, 2024 17:03
@kvid
Copy link
Collaborator Author

kvid commented Jun 4, 2024

@amotl - Thank's for a super quick approval. At which platform(s) did you test the installation? Or did you just review the changed line and approved it based on earlier experience that this should work without actually testing it?

@amotl
Copy link
Member

amotl commented Jun 4, 2024

Yeah, I did not explicitly test it. But in general, it looks good to me, and I assume you've tested it on your workstation?

@kvid
Copy link
Collaborator Author

kvid commented Jun 4, 2024

@amotl wrote:

Yeah, I did not explicitly test it. But in general, it looks good to me, and I assume you've tested it on your workstation?

As I wrote in the description at the top, I've tested it at my Windows-setup. However, I've not much experience in testing installation scripts, and I've asked for help to test at different platforms, so I would be glad if you can test at your platform(s) independently and verify that my change is sufficient. Please also simplify or otherwise improve my suggested test procedure.

About the added line in setup.py, do you know any better documentation of setup keyword arguments than https://docs.python.org/3.8/distutils/setupscript.html ? Is there a recommended or established usual order of the arguments?

@KarolNi
Copy link

KarolNi commented Jun 6, 2024

I was affected by #345. I executed:

pip uninstall wireviz
pip3 install git+https://github.com/wireviz/WireViz@fix345

And it fixed the error on my designs (I didn't test examples).

~/.local/bin/wireviz
pip 23.3.2 from /usr/lib/python3.12/site-packages/pip (python 3.12)
Python 3.12.3
Linux prc1 6.8.11-300.fc40.x86_64 #1 SMP PREEMPT_DYNAMIC Mon May 27 14:53:33 UTC 2024 x86_64 GNU/Linux

@kvid
Copy link
Collaborator Author

kvid commented Jun 7, 2024

@formatc1702 - maybe you can do a quick test similar to #347 (comment) at your macOS platform to verify that this fix is sufficient to install the templates also at your platform?

@formatc1702
Copy link
Collaborator

I have had a look at the files used to publish v0.4 to PyPI and can confirm that they are missing the templates/ directory.

I can also confirm that adding this fix resolves the issue.

My (simpler) test method:

  • run python -m build in the project root directory
  • check the contents of the .whl (actually a zip file) and .tar.gz files generated inside the dist/ directory.
    This is what I use to publish to PyPI.

Indeed, adding and removing the line in this proposed fix adds/removes the templates/ folder within the build files. I did not have to use include_package_data=True.

@formatc1702 formatc1702 merged commit 7115623 into release/v0.4.1-rc Jun 7, 2024
4 checks passed
@formatc1702 formatc1702 deleted the fix345 branch June 7, 2024 16:09
@kvid kvid removed the help wanted Extra attention is needed label Jun 7, 2024
kvid added a commit that referenced this pull request Jun 7, 2024
Specify all HTML files under templates folder
to be included as package data files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants