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

Highlight the nightly version needed to compile the examples #1582

Closed
wants to merge 1 commit into from

Conversation

JohnTitor
Copy link
Member

Readers didn't notice the examples need a specific nightly version. Indeed other versions can work, but it introduces some confusion like #1556 and #1581. This emphasizes the nightly version to use.
A partial revert of #1373.

Signed-off-by: Yuki Okushi jtitor@2k36.org

Signed-off-by: Yuki Okushi <jtitor@2k36.org>
@JohnTitor JohnTitor force-pushed the highlight-nightly-ver branch from 00643c7 to 1089f2d Compare February 4, 2023 01:19
@tshepang
Copy link
Member

tshepang commented Feb 4, 2023

To me, reports like that just indicate that we should update the code so that it builds again (with latest nightly). I would not call it confusion. I also think we should use CI to preempt such build failures.

One concern with this change is it may make someone install some old nightly without a need.

@JohnTitor
Copy link
Member Author

To me, reports like that just indicate that we should update the code so that it builds again (with latest nightly)

While I agree it should be up-to-date always, the internal API can be changed at any time and it may require updating so often. Our maintenance hands are limited, you can see untriaged date checks and a lot of open issues as proof of it. So pinning the nightly version and suggesting it should make some sense, we could make sure the examples work and readers can know how it works at least.

One concern with this change is it may make someone install some old nightly without a need.

What's the problem with it at all? rustup can help it and users can install it easily. Also, the examples already need to install additional components like rustc-dev, so I don't believe it makes much difference.

@JohnTitor
Copy link
Member Author

To begin with, the purpose of the example is to let the reader intuitively know how it works. Forcing readers to report every time it breaks would hinder that.
What's more, date-checks already encourage us to update regularly.

@tshepang
Copy link
Member

tshepang commented Feb 4, 2023

To me, reports like that just indicate that we should update the code so that it builds again (with latest nightly)

While I agree it should be up-to-date always, the internal API can be changed at any time and it may require updating so often. Our maintenance hands are limited, you can see untriaged date checks and a lot of open issues as proof of it. So pinning the nightly version and suggesting it should make some sense, we could make sure the examples work and readers can know how it works at least.

The old text already makes it clear which nightly is known to work, so that should cover it while we have not got to update the code.

One concern with this change is it may make someone install some old nightly without a need.

What's the problem with it at all? rustup can help it and users can install it easily. Also, the examples already need to install additional components like rustc-dev, so I don't believe it makes much difference.

It is waste, especially given how large toolchain downloads are. The new text is also misleading... it makes it look like no other nightly works.

@JohnTitor
Copy link
Member Author

The old text already makes it clear which nightly is known to work, so that should cover it while we have not got to update the code.

The current results in opening the issues linked above, I wouldn't call it clear in the face of such a situation.

It is waste, especially given how large toolchain downloads are.

That is a problem with the toolchain itself, not with running the example on the old nightly. Repeating myself but it already increases the size by using rustc-dev/llvm-tools components.

The new text is also misleading... it makes it look like no other nightly works.

It can, but how does the reader know the other versions? And how can we help readers by providing that information?
Again, the primary goal here is to get the reader to actually compile and run the example. There seems to be no problem in showing only one nightly version to accomplish this.

@tshepang
Copy link
Member

tshepang commented Feb 4, 2023

The old text already makes it clear which nightly is known to work, so that should cover it while we have not got to update the code.

The current results in opening the issues linked above, I wouldn't call it clear in the face of such a situation.

It does not look like confusion to me. We can always makes the text clearer... let me do a PR to that effect.

It is waste, especially given how large toolchain downloads are.

That is a problem with the toolchain itself, not with running the example on the old nightly. Repeating myself but it already increases the size by using rustc-dev/llvm-tools components.

There is no need to add even more to that. Also, it's not needed most of the time... these particular breakages do not happen every month. As mentioned, let me try do a more clear message in a PR.

The new text is also misleading... it makes it look like no other nightly works.

It can, but how does the reader know the other versions? And how can we help readers by providing that information? Again, the primary goal here is to get the reader to actually compile and run the example. There seems to be no problem in showing only one nightly version to accomplish this.

I expect that this is how it works... reader (hopefully) sees "this was tested with", tries their nightly since it might work, their nightly fails, then user (hopefully) remember that there was a "this was tested with" message, and only then do they download the tested nightly.

Note that, as already mentioned, we can have CI so that we detect such failures before users do. It does not address the concern about not having enough hands to keep things updated of course, but we also don't have to block merges on it failing.

@JohnTitor JohnTitor closed this Feb 9, 2023
@JohnTitor JohnTitor deleted the highlight-nightly-ver branch February 9, 2023 09:57
@JohnTitor
Copy link
Member Author

The PR here is meaningless according to @tshepang, closing.

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