[bug] GitHub Actions build_examples.py output not showing up

[formatc1702]

After testing #94, I realized that the output images referenced to in the main readme as well as the example and tutorial galleries (readme.md in the respective directories) are no longer showing up on GitHub in the feature branch.

I am not familiar with GitHub actions, but if I understand correctly, the new addition from a3f2134...

    - name: Upload examples, demos, and tutorials
      uses: actions/upload-artifact@v2
      with:
        name: examples-and-tutorials
        path: |
          examples/
          tutorial/

is meant to work as follows:

• Before commiting/pushing, the user runs python build_examples.py clean to remove any PNG, SVG, GV and other output files as well as the two readme's to keep the repo clean 👍
• after pushing to GitHub, this Action is supposed to generate the examples and readmes, and add them to the repo again?

Either I am misunderstanding, or it's not working; the fact is that the current system is broken.
Where exactly do the files uploaded by this action end up?

Perhaps @aakatz can explain what's going on, and maybe @flopp (who implemented the initial GitHub Actions workflow) knows something about this topic? Thanks!

[kvid]

I suggest reversing these changes to GitHub actions and test them in a separate test repo, e.g. a fork from this one.

[formatc1702]

I am leaning towards removing the examples from the repo entirely, and curating a second wireviz-examples repo... maybe via GitHub actions that separate repo could be updated on every commit? Something like this perhaps

[kvid]

Does this mean that we throw out the possability to do regression testing? You asked for comments about this to #94 and there I tried explaining my thoughts about this, but the case seems closed.

I'm a bit confused about the workflow. You seem to merge in many PRs rather quickly into a test feature branch and then do a review, but as the PR seems to disappear from the PR list after your merge-in, it will be hidden from people that might have comments unless they discover it before your merge-in. It is also hard to know when you decide it's good enough or if it has just been put on hold in your feature branch. In PR #96 I was also asked for comments, but you merged it in the next day just before my comments about the change requests from #17 that still was not applied. Now, I don't know if anyone care about comments or change requests because the discussion turned towards the new banded coloring.

I thought the PRs normally are kept open durimg reviews, updates, and until all change requests are corrected, and then closed by merge-in (normally to dev).

Please tell me what I have misunderstood about the workflow.

[formatc1702]

Thanks for your comment, I'll try and address the issues you mentioned!

Does this mean that we throw out the possability to do regression testing?

I agree this is a major drawback of the separate repo approach. I haven't found a solution yet, but I am looking into using .gitattributes (link 1, link 2) to at least make reviews and merges a bit easier if we keep the output files as part of the repo. I am very open to input regarding the issue.

You asked for comments about this to #94 and there I tried explaining my thoughts about this, but the case seems closed.

I will admit that I merged this a bit too quickly without realizing all the consequences. The hope is to get a satisfactory result via this new issue #100.

I'm a bit confused about the workflow. You seem to merge in many PRs rather quickly into a test feature branch and then do a review, but as the PR seems to disappear from the PR list after your merge-in, it will be hidden from people that might have comments unless they discover it before your merge-in. It is also hard to know when you decide it's good enough or if it has just been put on hold in your feature branch.

You raise very valid points. As a non-programmer and self-taught Git[Hub] user, I am figuring out all this stuff on the go.
While I can re-open issues that have been marked as closed, it seems that once I merge a PR into any branch on the main repo, it is marked as such, with no possibility to mark as still open for review post-merge, even if it was merged into its own feature branch.
I specifically did not merge #96 into dev since I wanted to try it out and add some tweaks (see 8e680ee and its commit message) beforehand.

In PR #96 I was also asked for comments, but you merged it in the next day just before my comments about the change requests from #17 that still was not applied.

See my last sentence above and the linked commit. I was eager to get the feature in, so I decided it was easier to apply those missing minor changes myself.
I also decided to remove the bgcolor stuff because it was implemented within the multicolor wire PR without any input from me or others whether this was actually desired. I am planning to open a new issue to discuss some coloring options, e.g. also to invert colors to make the graphs look better when Hackaday writes about WireViz 😉 .

Now, I don't know if anyone care about comments or change requests because the discussion turned towards the new banded coloring.

That is unfortunate and I have asked for a new issue to discuss the color bands, so as not to derail from the original topic.

I thought the PRs normally are kept open durimg reviews, updates, and until all change requests are corrected, and then closed by merge-in (normally to dev).

Please see my answer three paragraphs up. Maybe I am missing some way of getting the changes from a PR into my local copy (on its separate feature branch) without marking it as officially "merged" until it reaches the dev branch. 🤔

Please tell me what I have misunderstood about the workflow.

If anything, you are overestimating the existence of a defined workflow 😄 I very much appreciate receiving your input and want to keep the discussions open; I hope that this intention is conveyed here as well as in all the other threads!

[kvid]

Thank you for addressing all the issues in my long posting. I have never got the impression that you deliberately ignore or want to avoid comments, but by closing the PRs too early, you risk that fewer people are seeing the commits and comments, and hence risk loosing valuable feedback and maybe the PR author might ignore comments on his closed PR.

I am looking into using .gitattributes (link 1, link 2) to at least make reviews and merges a bit easier if we keep the output files as part of the repo. I am very open to input regarding the issue.

I have never used linguist-generated=true myself, but from the link 2 description, it seems to be a useful feature in our case.

Please read my 3 comments to #94 and argue against me before you decide what to do with the generated files.

I would suggest testing the GitHub actions thoroughly in a separate repo before applying it in your main repo (this). Automatically regenerate all output files for each PR might not be what you want unless at least all output files get identical contents and that git detect that when the commits do not change the output. Note that different Graphviz versions normally generate different output from the same input.

I recommend you keep the PRs open until there are nothing more to do and you have merged in all or the useful parts, or finally rejected it. You can fetch the PR commits into a local branch from your git command for testing while keeping the PR open. This also makes it easy to rebase or cherry-pick the commits onto other branches in your local repo to prepare pushing them to GitHub.

Personally, I use remote URLs to several forks from my local repo and gitk to manage all the branches:

(wv-env) kvid@PC:~/src/formatc1702/WireViz/src/wireviz$ git remote -v
aakatz3 https://github.com/aakatz3/WireViz.git (fetch)
aakatz3 https://github.com/aakatz3/WireViz.git (push)
kvid    git@github.com:kvid/WireViz.git (fetch)
kvid    git@github.com:kvid/WireViz.git (push)
origin  https://github.com/formatc1702/WireViz.git (fetch)
origin  https://github.com/formatc1702/WireViz.git (push)
(wv-env) kvid@PC:~/src/formatc1702/WireViz/src/wireviz$ git fetch aakatz3
(wv-env) kvid@PC:~/src/formatc1702/WireViz/src/wireviz$ git fetch origin
remote: Enumerating objects: 18, done.
remote: Counting objects: 100% (18/18), done.
remote: Compressing objects: 100% (7/7), done.
remote: Total 18 (delta 9), reused 16 (delta 9), pack-reused 0
Unpacking objects: 100% (18/18), done.
From https://github.com/formatc1702/WireViz
 * [new branch]      feature/doctype-test       -> origin/feature/doctype-test
   feee3c0..435f6b9  feature/multicolor-wires-2 -> origin/feature/multicolor-wires-2
(wv-env) kvid@PC:~/src/formatc1702/WireViz/src/wireviz$ gitk --all &

However, there are other methods that might work better for you. Here are some links you might find useful:
https://stackoverflow.com/questions/27208042/neat-clean-compacted-git-pr-and-merge-commit-into-one-commit
kahmali/meteor-restivus#89 (comment)
https://gist.github.com/adam-p/15413fabef6cffecd897
https://mattstauffer.com/blog/how-to-merge-only-specific-commits-from-a-pull-request/
https://www.thedroidsonroids.com/blog/splitting-pull-request

Note that I have not tested all these methods myself.

[formatc1702]

Thanks for all the links! They all offered some new insight, but the first one gave me exactly what I [didn't know I] needed:

First, in your repository, find the [remote "origin"] section of the .git/config file and add this line:

fetch = +refs/pull/*/head:refs/remotes/origin/pr/*

Make sure to add it BEFORE the existing fetch line. Now, every time you git fetch, you'll get all the Pull Requests in the repo updated! This is a one-time change that will give you direct access to PRs forever.

Previously, I was doing something similar to your suggestion, tracking various forks, but it was a bit messy since I don't really need to see everything other people are doing, until it becomes a PR. Now I've learned I can just check those out locally while keeping the PR open, and my commit tree looks exactly how I need it! (Yes I use a GUI, sue me 😆)

The only downside is that all the PR branches persist even after the PR is closed...
[Edit] Maybe this will solve that issue?

Thanks so much for the hints!

Comments on the regression testing coming in a separate comment.

[formatc1702]

Regarding the output files:

• I agree doing diffs for regression testing is a good idea.

• Probably the biggest annoyance is the differing GraphViz version causing unneeded diffs.

  • One option is to have the clean option delete the .gv files, since they are of little interest after development of a feature is finished.

• Regarding the use of a _built directory or similar: I don't understand the first of your points:

  Two advantages by copying the YAML file to the subfolder first:

  • Avoid changes to the Harness class.
  • Avoid bad references from the readme files.

  What do you mean by avoiding changes to the Harness class?

[formatc1702]

As for specific steps to take now, to move the project forward while we figure this out, here's my proposal:

• I will build the examples in the feature/multicolor-wires-2 branch, and merge it into dev as usual, so the READMEs work properly.
• I will disable the new GitHub Action and test it in a separate repo.

This will allow me to work on #66, #69, #77, #78 and hopefully resolve #95 soon, using the newest dev branch, and move towards an interim release of v0.2.
All other open features and issues shall be implemented afterwards in version >= v0.3

[kvid]

I'm glad you found my links helpful.

Right now, I have only time to answer one of your messages (I'll look into the rest later):

Regarding the output files:

• I agree doing diffs for regression testing is a good idea.

Thank you.

• Probably the biggest annoyance is the differing GraphViz version causing unneeded diffs.

I agree, but as long as we diff the .gv files, then we probably can safely ignore any diagram diffs. I want to look into that when I get the time.

• One option is to have the clean option delete the .gv files, since they are of little interest after development of a feature is finished.

I disagree. The .gv file does not change with the Graphviz version and is very useful for regression testing. It's mainly the .svg and .png outputs that cause problems in that context.

• Regarding the use of a _built directory or similar: I don't understand the first of your points:

  Two advantages by copying the YAML file to the subfolder first:

  • Avoid changes to the Harness class.
  • Avoid bad references from the readme files.

  What do you mean by avoiding changes to the Harness class?

If we want to write the generated files to a subfolder without first copying the YAML file to the same subfolder, I believe methods for writing output files in the Harness class need to be changed to allow a different destination folder, but I might have overlooked something. It is not tested.

[flopp]

My intent with this Github action is not to recreate the example images e.g. for the Readme or the examples folder, but to run some code that uses the WireViz API as a basic test (e.g. to catch syntax errors, API misuse, etc.).
I guess one should create some proper unit tests for that...

[aakatz3]

That action is not intended to automatically re-add the images to the repo, I do not think that is possible, and it would create an infinite loop in the actions. It is intended to allow the generated examples to be downloaded and verified, so that the output generated by the github actions can be compared to the expected output, as the old action without the artifact upload simply generated and then discarded the examples.

[kvid]

Thank you for bringing the discussion back on track. I'm sorry for including the off-topic issues here. Is it possible to move some messages to another issue (new or existing)?

[formatc1702]

I've opened #101 to allow for general discussion among collaborators :)
I'll see if I can move some comments there later today.

It seems I misunderstood the use of the new GitHub action, and it requires further review, preferably as a separate issue.

In the interest of moving the project forward, as mentioned earlier, I propose to keep including the rebuilt examples for now so as not to block new feature merges. If there are no major objections, I will close this issue soon.

[kvid]

I have no objections.

[aakatz3]

I have no objections, the idea was to try to prevent any rebuilt examples from creating merge conflicts.