<cr1901_modern>
sb0: Did you check the most recent commits on that PR? For some reason Github's didn't mark stuff as resolved when I updated.
<cr1901_modern>
Anyways, the crux of that PR is that tests shouldn't fail if you don't have proprietary toolchains installed. Which they will if you run build(source=True). But the tests should be run. >>
<cr1901_modern>
So _if_ a given test fails because the toolchain isn't installed, I rerun the test with source=False. Because build isn't idempotent, I have to recreate the toolchain object.
<cr1901_modern>
if the build fails*.
<cr1901_modern>
sb0: Alternate idea... raise unittest.SkipTest if toolchains aren't installed. That should make it simpler.
<sb0>
cr1901_modern: yes, it's still complicated
<sb0>
why is it more than a few if's?
<cr1901_modern>
Because you potentially have to run platform.build() twice
<sb0>
why?
<cr1901_modern>
Xilinx, ISE, and Diamond can raise an exception if the toolchain_path doesn't exist. THe other toolchains don't have this problem.
<cr1901_modern>
So I either handle the exception (rerun platform.build()) or the test fails.
<cr1901_modern>
Or alternately I could skip the test.
<sb0>
can you explain the logic of what you are trying to do?
<cr1901_modern>
Python tests fail if they raise an exception, right? Most test exceptions indicate that the last committer broke the code. IMO the exception raised from platform.build() when toolchains don't exist is spurious; it shouldn't cause CI to fail.
<sb0>
and why the tests should care whether the toolchain has this sourcing script or not?
<sb0>
what we do with artiq is set certain environment variables that e.g. say that the hardware is present
<sb0>
otherwise the corresponding tests are skipped
<sb0>
can't the same thing be done here?
<cr1901_modern>
That'll work too.
<cr1901_modern>
>and why the tests should care whether the toolchain has this sourcing script or not?
<cr1901_modern>
And to answer this question... ummm I guess it doesn't have to.
<cr1901_modern>
My logic here was: "If sourcing is enabled, anticipate a spurious exception. If it's disabled, any exception raised should fail the test."
<cr1901_modern>
Your idea is cleaner tho.
<cr1901_modern>
sb0: TLDR I was trying to make the unittests smarter so the user didn't have to do manual work
<sb0>
the exception may not be that spurious and also the user wants to know if the toolchain gets tested or not
<cr1901_modern>
So toolchain tests (for the proprietary ones anyway) should be opt-in?
<whitequark>
cr1901_modern: do not run platform.build twice
<whitequark>
in current migen this can mess up the gateware
<whitequark>
because of finalization quirks
<whitequark>
iirc i hit it in glasgow and had to monkey patch a workaround
<cr1901_modern>
even if I recreated the platform object?
<whitequark>
just avoid it
<whitequark>
nmigen is designed to not have this problem
<cr1901_modern>
what form do artiq vars take? /me takes a look
<cr1901_modern>
and well MIGEN_HAS_QUARTUS if that backend ever gets love
<cr1901_modern>
Looks like MIPS realized they lost the war...
<whitequark>
they were at war?
<whitequark>
i think they were asleep.
<cr1901_modern>
Maybe they were neutral, hoping that their router contracts would be enough to get by. I dunno. My gut feeling is "too little too late", though I'm not against a litigation-proof open MIPS core.
<sb0>
cr1901_modern: yes, disable the toolchain test unless some environment variable is set
<sb0>
why do you care about mips? use lm32, risc-v or even or1k
<whitequark>
mips went open-source
<whitequark>
that doesnt make it worth anything
<sb0>
cr1901_modern: in general people have only one shitty FPGA toolchain installed, which is enough pain already
<cr1901_modern>
Yea I seem to be an outlier there
<cr1901_modern>
I have 3
<cr1901_modern>
(on different machines)
<cr1901_modern>
sb0: I don't care about MIPS, just... the more RISCy CPUs the merrier lol.
<whitequark>
not really
<whitequark>
we do not need more shitty CPUs like mips
_whitelogger has joined #m-labs
<cr1901_modern>
sb0: >yes, disable the toolchain test unless some environment variable is set
<cr1901_modern>
Since we don't actually build, toolchain tests without source variable are unaffected if the toolchain isn't present. Do you want me to include env vars for those too?
cr1901_modern1 has joined #m-labs
cr1901_modern has quit [Disconnected by services]
cr1901_modern1 has quit [Client Quit]
cr1901_modern has joined #m-labs
rohitksingh has joined #m-labs
<sb0>
cr1901_modern: if the toolchain is installed, then it should build. if it's not, do nothing.
<cr1901_modern>
ack. Give me a day or so to rework the tests then
rohitksingh has quit [Ping timeout: 250 seconds]
rohitksingh has joined #m-labs
_whitelogger_ has joined #m-labs
_whitelogger has quit [Ping timeout: 268 seconds]
<_whitenotifier-6>
[m-labs/nmigen] whitequark pushed 1 commit to master [+0/-0/±1] https://git.io/fhJK1