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

fix parsing adjacent hyphens in a literal #616

Merged
merged 3 commits into from
Jan 26, 2020
Merged

fix parsing adjacent hyphens in a literal #616

merged 3 commits into from
Jan 26, 2020

Conversation

ktbarrett
Copy link
Contributor

Addresses #529 by improving the comment removal regular expression. But this time it actually works!

Will rebase into a single commit tonight.

@ktbarrett ktbarrett marked this pull request as ready for review January 23, 2020 18:09
@kraigher
Copy link
Collaborator

I am ready to merge this if the checks pass. Do you need any help on how to run the checks locally?

@kraigher
Copy link
Collaborator

I saw that you pushed a fix the same second I wrote the comment, good :-)

@ktbarrett
Copy link
Contributor Author

ktbarrett commented Jan 23, 2020

@kraigher I do not have the current master or black locally to check. I looked at the checks to see where it failed. If it is more work than that, I will have to wait until tonight.

EDIT: Yeah, looks like black is stil complaining. I'll get everything in order by tomorrow.

@kraigher
Copy link
Collaborator

You should be able to run black using just:

> python -m pip install tox
> python -m tox -e py37-fmt

Where you can replace py37 which whatever version you have.

@ktbarrett
Copy link
Contributor Author

> python -m pip install tox

I can't download anything where I am. If you want this in today, feel free to make any changes.

@eine
Copy link
Collaborator

eine commented Jan 23, 2020

@ktbarrett, if this is just an issue with running black once, I'll pick it later. Otherwise, I'll push the format fix and leave it open.

@eine
Copy link
Collaborator

eine commented Jan 23, 2020

I pushed black fixes, but CI jobs seem to be stalled regardless. These are the last three attempts:

@ktbarrett
Copy link
Contributor Author

@eine Thanks.

I'm home now. How do I recreate the environment the regressions run in? I'm assuming there is some assumed set of tools the regression work with. Just running tox locally leads to all the acceptance tests failing. And is there anyway to increase the verbosity of the tests while they are running? What I have now is not enough to go forward.

@eine
Copy link
Collaborator

eine commented Jan 24, 2020

I'm home now. How do I recreate the environment the regressions run in? I'm assuming there is some assumed set of tools the regression work with. Just running tox locally leads to all the acceptance tests failing.

The environment just requires a simulator (GHDL is used in the CI workflow) and tox. Then, execute tox -e py38-acceptance-ghdl in the root of the repo and it will create a virtualenv and install all the dependencies. If you have docker/podman, you can use the following command to run a container (instead of installing GHDL on your host):

docker run --rm -tv $(pwd):/src -w /src ghdl/vunit:llvm bash -c "pip3 install tox; tox -e py38-acceptance-ghdl"

And is there anyway to increase the verbosity of the tests while they are running? What I have now is not enough to go forward.

Tests are in subdir tests. However, I suggest to run all of them, and then copy the full name of the ones you want to further test. In tox.ini you will find the pytest command. For example:

pytest -v tests/acceptance/test_artificial.py::TestVunitArtificial::test_artificial_verilog

However, it seems to be failing when testing external run scripts. These are, indeed, the examples. Hence, you can first try to execute each example individually.

@ktbarrett
Copy link
Contributor Author

The old pattern was correct, but really slow, so I optimized. I'm running raw times master vs this PR to see the effect on performance.

@ktbarrett ktbarrett closed this Jan 24, 2020
@ktbarrett ktbarrett reopened this Jan 24, 2020
@eine
Copy link
Collaborator

eine commented Jan 24, 2020

All tests pass and there seems to be no time penalty any more. However, no specific test is added. That'd be interesting to prevent future regressions. Anyway, @ktbarrett, I'm ok with merging if you confirm that the examples you were trying in #529 do work now.

@ktbarrett
Copy link
Contributor Author

@eine I have confirmed that these fix my issues locally. I can add a unit test to the regression as well.

@eine
Copy link
Collaborator

eine commented Jan 24, 2020

Thanks. Please, go ahead on this same PR.

@eine eine changed the title Fix comment parsing error fix parsing adjacent hyphens in a literal Jan 26, 2020
@eine eine merged commit 53f681c into VUnit:master Jan 26, 2020
@ktbarrett ktbarrett deleted the patch-1 branch January 26, 2020 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants