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

Attempt to roll tarball and Yosys packages for symbiflow-examples. Name the code blocks. Address issue #48. #50

Merged
merged 9 commits into from Sep 24, 2020

Conversation

tcal-x
Copy link
Contributor

@tcal-x tcal-x commented Sep 17, 2020

  • Remove extra git clone of repo (which had mixed in code from master)

  • Name all code blocks executed by tuttest

  • Use "bash -ex -"

  • Update conda packages and tarball to get IDELAYCTRL fix

  • Remove LOC constraint on IDELAYCTRL in Linux example RTL

  • Add 100T build for Linux example.

xc7/environment.yml Outdated Show resolved Hide resolved
@tcal-x
Copy link
Contributor Author

tcal-x commented Sep 18, 2020

Error is:

Writing bitstream ...
Traceback (most recent call last):
  File "/opt/symbiflow/xc7/conda/envs/xc7/bin/xcfasm", line 8, in <module>
    sys.exit(main())
  File "/opt/symbiflow/xc7/conda/envs/xc7/lib/python3.8/site-packages/xc_fasm/xc_fasm.py", line 55, in main
    fasm2frames(
  File "/opt/symbiflow/xc7/conda/envs/xc7/lib/python3.8/site-packages/xc_fasm/fasm2frames.py", line 186, in fasm2frames
    assembler.parse_fasm_filename(filename_in, extra_features=extra_features)
  File "/opt/symbiflow/xc7/conda/envs/xc7/lib/python3.8/site-packages/prjxray/fasm_assembler.py", line 190, in parse_fasm_filename
    raise FasmLookupError('\n'.join(missing_features))
prjxray.fasm_assembler.FasmLookupError: Segment DB RIOI3, key RIOI3.ILOGIC_Y0.ISERDES.MODE.MASTER not found from line 'RIOI3_X43Y39.ILOGIC_Y0.ISERDES.MODE.MASTER'
Segment DB RIOI3, key RIOI3.ILOGIC_Y0.ISERDES.NETWORKING.DDR.W8 not found from line 'RIOI3_X43Y39.ILOGIC_Y0.ISERDES.NETWORKING.DDR.W8 = 1'b1'
Segment DB RIOI3, key RIOI3.ILOGIC_Y0.ISERDES.NUM_CE.N1 not found from line 'RIOI3_X43Y39.ILOGIC_Y0.ISERDES.NUM_CE.N1 = 1'b1'
Segment DB RIOI3, key RIOI3.ILOGIC_Y1.ISERDES.MODE.MASTER not found from line 'RIOI3_X43Y45.ILOGIC_Y1.ISERDES.MODE.MASTER'
etc

@tcal-x
Copy link
Contributor Author

tcal-x commented Sep 18, 2020

Hmm, I guess I'm encountering issue #48 , so I can't see my new test run.

@litghost
Copy link
Contributor

Hmm, I guess I'm encountering issue #48 , so I can't see my new test run.

The CI is green now, but does this comment imply we are not testing the change in CI? I'm confused...

@tcal-x
Copy link
Contributor Author

tcal-x commented Sep 18, 2020

Hmm, I guess I'm encountering issue #48 , so I can't see my new test run.

The CI is green now, but does this comment imply we are not testing the change in CI? I'm confused...

Yeah, I don't fully understand the Travis flow, but it seems it clones the master repo at some point, not the candidate. Although it's confusing, since CI did fail for my earlier changes, so it must use some aspects of the candidate. Maybe it's just the tuttest execution that uses master.

I'm tempted to just ask to merge it, and watch and be ready to roll back if needed.

@litghost
Copy link
Contributor

litghost commented Sep 18, 2020

I'm tempted to just ask to merge it, and watch and be ready to roll back if needed.

Agree. We should definitely improve the commenting/naming in the travis.yml. What we have now is just a bunch of "unnamed0", etc. It is very hard to follow what is going on.

@mgielda You wrote https://github.com/antmicro/tuttest/ , but the current README for tuttest doesn't explain how to attach names to tests. Can you please create some examples in the tuttest README for naming code blocks? Thanks.

GitHub
Contribute to antmicro/tuttest development by creating an account on GitHub.

@litghost
Copy link
Contributor

@tcal I believe I understand what Travis is doing and we should fix it up.

Changes to make:

  • Add comments to each tuttest invocation
  • Change every bash - to bash -ex -. This enables bash command tracing and fail on error, which are both important.

I'm going to annotate what it is doing below:

There are 3 relevant tuttest invocations:

From https://github.com/SymbiFlow/symbiflow-examples/blob/6850c5785a6339b151468186d7b879118fa64c4d/.travis.yml#L20

tuttest README.rst unnamed0 | bash -

Get the latest conda installer and write to conda_installer.sh

From https://github.com/SymbiFlow/symbiflow-examples/blob/6850c5785a6339b151468186d7b879118fa64c4d/.travis.yml#L25

tuttest xc7/README.rst unnamed0 | bash -

Sets INSTALL_DIR to "/opt/symbiflow/xc7" and then installs conda to INSTALL_DIR, along with the packages from environment.yml. This step uses the environment.yml that Travis checked out! So changes to environment.yml are impacted here!!! This also uses the contents to the new README, so changes to the README do affect CI!!!!!

From https://github.com/SymbiFlow/symbiflow-examples/blob/6850c5785a6339b151468186d7b879118fa64c4d/.travis.yml#L28

tuttest xc7/README.rst unnamed1,unnamed2,unnamed3,unnamed4,unnamed5 | bash -

unnamed1 enters the conda enviroment from the previous step (good), but also checks out a copy of master symbiflow-examples (bad, boooo). What we should likely do here is split the cloning of symbiflow-examples into another snippet! There are couple of reasons to do this. The first, and most important is that unnamed0 already needed to have a copy of the repo cloned so it could have a environment.yml!!!!

@litghost
Copy link
Contributor

litghost commented Sep 18, 2020

My suggestion for the order of snippets should be:

  1. (Do not invoke in CI) Set INSTALL_DIR, and instruct the user to choose a writable location (e.g. ~/opt/symbiflow)
  2. (Invoke in CI) Download conda and install it to INSTALL_DIR. Use conda enviroment to install git.
  3. (Do not invoke in CI) Enter conda enviroment and clone symbiflow-examples and cd to symbiflow-examples
  4. (Invoke in CI) Create new clean conda enviroment using symbiflow-examples/environment.yml
  5. (Invoke in CI) Build xc7 / eos examples

@tcal-x / @mgielda Thoughts?

Because Travis already checks out symbiflow-examples, step 3 is not required, and can be skipped. Step 1 should be set to a writable location within the CI run folder (e.g. pwd/conda).

@tcal-x
Copy link
Contributor Author

tcal-x commented Sep 18, 2020

@tcal I believe I understand what Travis is doing and we should fix it up.

The first, and most important is that unnamed0 already needed to have a copy of the repo cloned so it could have a environment.yml!!!!

Yes -- this is wrong for the same reason for humans trying to follow the tutorial!

We should make the repo clone the first thing, and then skip that section in tuttest.

All the other things you mentioned make sense to me, and match to what I saw in the Travis log.

@pgielda
Copy link
Member

pgielda commented Sep 18, 2020

Well tuttest is effectively following the tutorial. Which originally was just short, did require cloning only at the very end to download examples (there was no requirements.txt, all was in instructions) and repo only contained the example designs. Also it was not targetting multiple FPGAs like it is now.

I agree with the fact that now it has to be fixed -- maybe lets just not include cloning in the instructions (assume its already cloned)

@tcal-x
Copy link
Contributor Author

tcal-x commented Sep 21, 2020

Hi @litghost , does it make sense for me to merge these changes now and keep an eye on the run, then address the tuttest updates separately?

@litghost
Copy link
Contributor

Hi @litghost , does it make sense for me to merge these changes now and keep an eye on the run, then address the tuttest updates separately?

We should probably update tuttest either as part of this PR, or in a new PR seperate from this one first.

@tcal-x
Copy link
Contributor Author

tcal-x commented Sep 22, 2020

  1. (Invoke in CI) Download conda and install it to INSTALL_DIR. Use conda enviroment to install git.

Hi @litghost , I'm not sure what you mean by "Use conda environment to install git"

@litghost
Copy link
Contributor

  1. (Invoke in CI) Download conda and install it to INSTALL_DIR. Use conda enviroment to install git.

Hi @litghost , I'm not sure what you mean by "Use conda environment to install git"

I guess the question is whether we should assume if a user has git installed or not. If we assume that the user just has to install conda, then should we use the git from conda rather than the system git? I don't care strongly one way or another, but if we want to minimize assumptions about stuff installed on the user system, I was thinking it made some sense to use the git from conda.

@mithro
Copy link
Contributor

mithro commented Sep 22, 2020

For now, I think we can assume someone has git installed.

@tcal-x tcal-x changed the title Attempt to roll tarball and Yosys packages for symbiflow-examples. WIP: Attempt to roll tarball and Yosys packages for symbiflow-examples. Sep 22, 2020
@tcal-x tcal-x force-pushed the rollnew branch 2 times, most recently from 768c8bb to 44e319f Compare September 22, 2020 22:32
Signed-off-by: Tim Callahan <tcal@google.com>
Signed-off-by: Tim Callahan <tcal@google.com>
Signed-off-by: Tim Callahan <tcal@google.com>
Signed-off-by: Tim Callahan <tcal@google.com>
Signed-off-by: Tim Callahan <tcal@google.com>
Signed-off-by: Tim Callahan <tcal@google.com>
Restore output redirect for synth step (deleted by accident).
Checking status w/ EOS-S3 updates .. DNM!
Change instructions around INSTALL_DIR.
Update formatting to rst not md.
More formatting fixes, mostly for ..code.
Now use $INSTALL_DIR/xc7 and $INSTALL_DIR/eos-s3.
Formatting & instructions update.

Signed-off-by: Tim Callahan <tcal@google.com>
Signed-off-by: Tim Callahan <tcal@google.com>
@tcal-x tcal-x changed the title WIP: Attempt to roll tarball and Yosys packages for symbiflow-examples. Attempt to roll tarball and Yosys packages for symbiflow-examples. Name the code blocks. Address issue #48. Sep 24, 2020
Signed-off-by: Tim Callahan <tcal@google.com>
Copy link
Contributor

@litghost litghost left a comment

Choose a reason for hiding this comment

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

LGTM

@litghost litghost merged commit fc73681 into chipsalliance:master Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants