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.

Sign up for GitHub

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

Jump to bottom

most of the run-make tests could be silently passing on cases that should fail #13746

Closed
pnkfelix opened this issue Apr 25, 2014 · 2 comments · Fixed by #14000
Closed

most of the run-make tests could be silently passing on cases that should fail #13746

pnkfelix opened this issue Apr 25, 2014 · 2 comments · Fixed by #14000

Comments

@pnkfelix
Copy link
Member

Makefiles unfortunately do not error on undefined variable references; they just silently expand into an empty string. And an empty string is also not an invalid command in a recipe.

So, a recipe like this (observe highlighted line): run-make/dylib-chain/Makefile

does not actually work because of this: run-make/tools.mk

because $(call FAIL,main) is going to expand into an empty string; what was wanted was $(call FAILS,main)

Although it looks like most of the tests are using $(call FAIL,xxx), so its probably simpler to just change tools.mk to use the name FAIL instead of FAILS:

% ack FAIL
bootstrap-from-c-with-green/Makefile
11: $(call FAIL,main)

bootstrap-from-c-with-native/Makefile
10: $(call FAIL,main)

c-dynamic-dylib/Makefile
13: $(call FAIL,bar)

c-dynamic-rlib/Makefile
13: $(call FAIL,bar)

c-link-to-rust-dylib/Makefile
9:  $(call FAIL,bar)

c-static-dylib/Makefile
10: $(call FAIL,bar)

dylib-chain/Makefile
12: $(call FAIL,m4)

prefer-dylib/Makefile
8:  $(call FAILS,foo)

tools.mk
13:FAILS = $(TARGET_RPATH_ENV) ( $(RUN_BINFILE) && exit 1 || exit 0 )

(The bigger question is whether attempting to implement this fix is going to actually expose latent bugs...)
@pnkfelix pnkfelix changed the title most of the run-make tests are silently passing on cases that should fail most of the run-make tests could be silently passing on cases that should fail Apr 25, 2014
pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 25, 2014
@pnkfelix
run-make tests must distinguish between HOST_RPATH and TARGET_RPATH.
d89b98a 
What this commit does:

* Pass both of the (newly introduced) HOST and TARGET rpath setup vars
  to `maketest.py`

* Update `maketest.py` to no longer update the LD_LIBRARY_PATH itself
  Instead, it passes along the HOST and TARGET rpath setup vars in
  environment variables `HOST_RPATH_ENV` and `TARGET_RPATH_ENV`

* Cleanup: Distinguish in tools.mk between the command to run (`RUN`)
  and the file to generate to drive that command (`RUN_BINFILE`).  The
  main thing this enables is that `RUN` can now setup the
  `TARGET_RPATH_ENV` without having to dirty up the runner code in
  each of the `run-make` Makefiles.

I should have tried to fix rust-lang#13746 but I got distracted, and decided
fixing that is orthgonal to the changes in this branch of work.

Note that even with these changes, `make check-stage1` still does not
work all the way to completion.  (It just gets further than it did
before, but still breaks down in the `run-make` tests.)
pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 25, 2014
@pnkfelix
Fix syntactic mistake in earlier definition of FAILS.
f716518 
(Note that most tests are not using FAILS anyway, due to rust-lang#13746.)
@brson
Copy link
Contributor

brson commented Apr 25, 2014

Yuck. Nice catch.

@alexcrichton alexcrichton mentioned this issue Apr 25, 2014
Closed
@pnkfelix pnkfelix mentioned this issue May 7, 2014
Merged
@pnkfelix
Copy link
Member Author

pnkfelix commented May 7, 2014

part of #8058

bors added a commit that referenced this issue May 18, 2014
@bors
auto merge of #14000 : pnkfelix/rust/fsk-fix-issue13732, r=alexcrichton
e1403e1 
Fix #13732.

This is a revised, much less hacky form of PR #13753

The changes here:

 * add instrumentation to aid debugging of linkage errors,
 * fine tune some things in the Makefile where we are telling binaries to use a host-oriented path for finding dynamic libraries, when it should be feeding the binaries a target-oriented path for dynamic libraries.
 * pass along the current stage number to run-make tests, and
 * skip certain tests when running atop stage1.

Fix #13746 as well.
arcnmx pushed a commit to arcnmx/rust that referenced this issue Dec 17, 2022
@bors
Auto merge of rust-lang#13746 - feniljain:fix_extract_function, r=jon…
4596847 
…as-schievink

fix: make make_body respect comments in extract_function

Possible fix for rust-lang#13621

### Points to help in review:

- Earlier we were only considering statements in a block expr and hence comments were being ignored, now we handle tokens hence making it aware of comments and then preserving them using `hacky_block_expr_with_comments`

Seems like I am not able to attach output video, github is glitching for it :(
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Get make check-stage1 working again pnkfelix/rust
2 participants
@brson @pnkfelix