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

Remove parent's DEJAGNU environment variable #1377

Closed
wants to merge 1 commit into from

Conversation

a4lg
Copy link
Contributor

@a4lg a4lg commented Nov 28, 2023

make report / make check on riscv-gnu-toolchain depends on the default installation of DejaGnu's riscv-sim.exp.

If an user overrides the DejaGnu environment with custom riscv-sim.exp, many tricks used in the testing framework in riscv-gnu-toolchain stops working.

I noticed this issue while testing riscv-gnu-toolchain with an environment which I use custom DejaGnu configuration to test GCC (alone) with QEMU + custom arguments (written in $DIR/boards/riscv-sim.exp where DEJAGNU=$DIR/site.exp).

So, this commit removes parent's DEJAGNU environment variable by unexporting it.

Note that unexport is a feature of GNU Make and (before approving) check whether using a "not in POSIX make" feature is appropriate for this project.

`make report` / `make check` on riscv-gnu-toolchain depends on the
default installation of DejaGnu's `riscv-sim.exp`.  If an user overrides
the DejaGnu environment with `riscv-sim.exp`, many tricks used in
the testing framework in riscv-gnu-toolchain stops working.

So, this commit removes parent's `DEJAGNU` environment variable by
`unexport`ing it.
@cmuellner
Copy link
Collaborator

I wonder who is setting DEJAGNU so that we have to unexport it.

If this is only needed in a specific local environment, then a local wrapper script that calls make would be the best place to handle this. I don't think that non-default environments need to be supported if this can easily be done outside of this repo.

So I think it might be better to add a subsection in README.md to document the requirement and how to address potential issues.

@cmuellner
Copy link
Collaborator

After revisiting this, I still prefer not to merge this PR because it adds code that only prevents issues in certain environments. This PR would also prevent overriding DEJAGNU for cases where this is needed (for whatever reasons—see also https://www.gnu.org/software/dejagnu/manual/Customizing-DejaGnu.html).

In this particular case, it might be best to place the unexport in a local wrapper script for building toolchains. Or setting the DEJAGNU variable could be done in a wrapper script that is used for the other mentioned use-case ("test GCC (alone) with QEMU + custom arguments").

@cmuellner cmuellner closed this Apr 5, 2024
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.

2 participants