-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update the documentation for setting up dev environment for Ubuntu #1207
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merci pour cette mise à jour. Il y aurait quelques petits changements à apporter. Aussi, la branche a 4 commits, donc 2 merge commits, il faudrait éviter ça. Nous avons des branches linéaires, donc on encourage l'utilisation du rebase plutôt que des merge commits. Pour régler ça, vous pouvez commencer par faire un rebase sur la branche main
d'upstream (git rebase main
en supposant que main
est sur l'upstream, mais comme votre branche se nomme aussi main, faites attention). Ensuite, un rebase interactif vous permettra de combiner les commits restants (git rebase -i HEAD~3
par exemple).
docs/setupDevEnvironmentUbuntu.md
Outdated
git checkout <to the commit before the last> | ||
``` | ||
|
||
In CMakeList.txt, comment these lines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ici, il serait préférable de dire que si l'erreur "blabla" survient à la compilation. Vous pouvez ajouter un lien à cet issue, que j'ai enfin retrouvé: Project-OSRM/osrm-backend#6704 et qui explique le workaround que vous mentionnez, mais en mettant le lien à l'issue on pourra éventuellement voir si c'est réglé.
docs/setupDevEnvironmentUbuntu.md
Outdated
yarn migrate | ||
yarn create-user | ||
``` | ||
When creating a user, provide an email address as a backup in case the userID doesn't work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Que voulez-vous dire par "in case the userID doesn't work"? Plutôt "in case of login failure, to get a new password". Aussi, le ton pourrait être moins impératif "you may provide an email"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, c'était bien un login failure. Après avoir créé mon utilisateur, je n'arrivais pas à me connecter avec le userID, mais ça fonctionnait en utilisant l'adresse courriel. J'ai ajusté la documentation!
Suggestions pour le commit message: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C'est bon, vous pouvez mettre à jour le commit message considérant les commentaires de Yannick. Sinon, pour le contenu, c'est parfait
2e40f5d
to
644758e
Compare
Prêt à merger? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaligrafy tu as ajouté un merge commit dans la branche
Juste utilisé le bouton automatique de github comme on fait souvent non? |
Tu n'as probablement pas vérifier si c'était rebase ou merge. |
Il restait à mettre à jour le commit message avec les informations demandées |
En fait, c'est plutôt dans le commit message qu'il aurait fallu mettre à jour, mais peut-être qu'avec un squash and merge, je peux éditer le commit final |
Update the documentation for setting up dev environment for Ubuntu 24.04 located in the
docs
foldersetupDevEnvironmentUbuntu20.04.md
tosetupDevEnvironmentUbuntu.md